"Rob Stewart" <stewart@sig.com> wrote in message: Continuing where I left off yesterday.... Again, I'll only comment on the parts where I disagree, don't understand or believe more explanation is necessary.
streambuf_facade::streambuf_facade
You have two sections with the same heading;
I'm emulating the Dinkumware documentation style. For overloaded functions, there is a heading like basic_istream::operator>> followed by a recapitulation of the relevant part of the synopsis, followed by a decription of each member. I've found the documentation hard to read when I group too many overloads together, since I end up writing stuff like "this seventh member constructs a ..." So I split them up, but give them headers with the same name. Perhaps heading like streambuf_facade::streambuf_facade (I) streambuf_facade::streambuf_facade (II) streambuf_facade::streambuf_facade (III) or streambuf_facade::streambuf_facade (Default) streambuf_facade::streambuf_facade (Forwarding) streambuf_facade::streambuf_facade (From Policy) would be better in cases like this.
both should fall under the heading "Constructors." Otherwise, "streambuf_facade::streambuf_facade" should be written as "streambuf_facade<T,Tr,Alloc,Mode>::streambuf_facade."
I don't see how adding the template arguments helps.
______ The first streambuf_facade::streambuf_facade
"Before the instance can be used for i/o, one of its..." ^^^^^^^^ streambuf ______ The second streambuf_facade::streambuf_facade
"Constructs a streambuf_facade which is ready to perform i/o, where the parameters have the following interpretations:" ^^^^^^^^^^^^^^^ meaning
You don't call out function parameters like this many other places. There's nothing wrong with it, but you should choose to explain them in a paragraph or a list and do that throughout the documentation.
I choose a table explaining the parameters only when I thought it would be the more helpful format. There are cases where it seems unnecessary, e.g., mapped_file_sink(const std::string& path); Constructs a mapped_file_sink to access the file with the given pathname. path - the pathname of the file to be accessed.
______ streambuf_facade::open ^^ <T,Tr,Alloc,Mode>::
Again, adding the template arguments just makes the headers harder to read, IMO.
"Assocaites the given instance of T with this instance of ^^^^^^^^^^ Associates
streambuf_facade, if there is no such instance currently ^^^^ T
I don't understand the ^^^^ under 'there is'. I guess I'm viewing your corrections with an incompatible font.
______________ Class template stream_facade ______ Examples
Where are the streambuf examples? You said they were the "fundamental components" of the library.
Good point. Based on the comments of reviewers, I plan to 1. explain early in the docs the roles of stream buffers and streams in the standard library i/o framework 2. explain that the streams in the current library are provided only as a convenience 3. Show how to use an arbitrary streambuf with a plain istream or ostream 4. Explain that most of the examples will use streams, since these are more familiar and will probably represent the most comon use of the library.
_________________________________________________________________ filtering_streams.html _______________________________ Synopsis
There are two, identical declarations of boost::io::filtering_streambuf.
Thanks, the second should be filtering_stream.
_______________________________ Examples
Do any of these techniques apply to filtering_streambufs?
They all apply to the streambuf variants of the stream templates used.
_________________________________________________________________ code_conversion.html
"* converting_streambuf: A wide-character stream buffer template having an interface essentially identical to
Don't break the paragraph here.
Seems to be a mozilla bug.
filtering_streambuf, with the following additional property: after zero or more wide-character filters have been added to the chain, a sequence of zero or more narrow-character filters and a narrow character resource may be added. A converter is inserted automatically between the final wide-character filter and the first narrow-character filter or resource."
Rephrasing that will simplify it and make it clear:
...essentially identical to filtering_streambuf, which automatically inserts a converter between the last wide character filter, if any, and the first narrow character filter or resource. (Any wide character filters must precede the narrow character filters and resource in the chain.)
I agree that my wording is awkward, but I'm not sure I like your suggestion. How about essentially identical to filtering_streambuf except that both wide- and narrow-character filters and resources can be added to it. The only restriction is that the wide- character components must come first. When the first narrow-character components is added, the libray automatically inserts a code_converter.
_________________________________________________________________ convenience.html _______________________________ Overview
You mention a convenience function named "copy" but the only function shown is operator <<.
I agree this looks funny, and will have to be changed. Refering to the supposedly familiar overload of basic_ostream::operator<< was supposed to be a quick way of explaining what copy does. But it clearly didn't work.
_________________________________________________________________ converter.html _______________________________ Description
"The class templates converter is...and produces a resource with ^^^^^^^^^ template
wide character type" ^^^^ a wide
How about: A resource adapter which which takes a narrow-character resource type and produces a wide-character resource type by introducing a layer of code conversion using a std::codecvt. ?
______________ converter::converter
Calling a constructor a member, however accurate, is odd. Why not just call it a constructor?
Good point. I thought I was following the Dinkumware convention, but having just checked, I see that I wasn't.
______________ converter::open
"Assocaites the given instance of Resource with this instance of converter, if there is no such instance currently associated; otherwise, throws std::ios_base::failure."
"Associates" is misspelled, but rephrasing it would work well:
Makes a copy of the Resource to be used by the converter. If the converter already has a Resource, it throws std::ios_base::failure.
I agree that associate/dissociate is awkward. How about attach/detach?
I don't think "associates" leaves out an important detail, suggesting that the Resource is tracked by pointer and that the Resource's lifetime must extend to the next call to close().
I'm planning an entire section on lifetime management issues.
"The third parameter determines the size of the buffer or buffers used for code conversion." ^^^ for the
My formulation sound more natural to me.
______________ converter::close
"Disassociates from this instance of converter any instance of the resource type Resource currently associated with it, calling cleanup functions as appropriate and destroying the associated instance of Resource."
This is awkwardly phrased. I suggest:
Destroys the converter's current Resource, if any, calling the cleanup functions of any filters and the Resource, as appropriate.
Detaches the converter's current Resource, if any, and destroys it, after calling any appropriate cleanup functions ?
I don't think "dissociate" (not "disassociates," BTW) is necessary.
Thanks. Google tells me that 'dissociate' is about twice as common as 'disassociate'. Maybe 'disassociate' should mean 'insult an attorney who hasn't made partner.'
_________________________________________________________________ exceptions.html _______________________________ The Boost Iostreams Library ______________ Policy
The text is formatted differently than on other pages for this heading level.
I used H5 here because I thought the higher levels looked odd. But I think it's a moot point because I should move the rationale to the central Rationale section.
_______________________________ Exception Safety
"1. Resources are freed by calling destructors or close, if appropriate."
Don't break the line between "or" and "close."
Mozilla -- up to her old tricks.
"Conditions 2. and 3. rely on the specification of the filter and resource concepts. Note that there is no guarantee that a stream's or stream buffer's character sequences are in a consistent state, i.e., that data has not been corrupted, and no way in general to determine the state of these sequences after an exception."
After reading this paragraph, I'm left to wonder whether removing and replacing the resource, as described in 3.b, means that the state of the character buffers is well-defined or still unknown as described here. IOW, this paragraph seems to contradict 3.b and should be rephrased to clarify how the state of the buffers can be known and corrected.
Good point. After an exception, and before the chain is modified, there is no guarantee.
_______________________________ Acknowledgments
"Thanks to Angelika Langer and John Torjo for discussion of exceptions."
What does "for discussion of exceptions" mean? Did you have conversations with them on the subject? Then you need to write, "for our discussions on exceptions and exception handling."
Yes, this sounds awkard. How about: Angelika Langer and John Torjo provided helpful comments on the role of exceptions in the standard iostreams library. ?
_________________________________________________________________ regex_filter.html
_______________________________ Reference ______________ basic_regex_filter::formatter
"The type of object which a basic_regex_filter uses to...." ^^^ formatter is the
Do you feel the same way about elision of the subject in function documentation which begins with "Returns"?
"Since Boost.Function objects are implictly constructible from function objects with the correct signature, users of regular expression filters may define their own function objects with the correct signature and pass them to the basic_regex_filter constructor..."
This is incomplete. One can write free functions, lambda functions, etc. that satisfy Boost.Function to create formatters.
How about: users of regular expression filters may pass any function or function object to the basic_regex_filter constructor as long as its signature is compatible. ?
______________ basic_regex_filter::basic_regex_filter
"replace - A function which will be passed each match_results object in succession and whose return values will be be used as replacement text"
Rewording this will make it clearer:
A Boost.Function object to which each regular expression match is passed. The return value is used as the filter's result in lieu of the matched text.
How about: A Boost.Function object to which each regular expression match is passed. The return value will be used in the filtered character sequence in lieu of the matched text. ?
_______________________________ Example
"See Examples." ^^^^^^^^
I'm not sure I see the problem. Maybe: See Examples (Regex OutputFilter).
_________________________________________________________________ newline_filter.html _______________________________ Description
"The class templates basic_newline_filter is a DualUseFilter ^^^^^^^^^ template
which converts between the text file formats used by...." ^^^^^^^ among
Why? I hope it's not "the superstition that 'between' can be used only of the relationship between two things, and that if there are more 'among' is the right preposition." (Fowler) :-)
"Note: It is not known if specializations of basic_newline_filter other than basic_newline_filter<char> are useful. If not, the template parameter will be eliminated."
Make this a footnote referenced from "Ch" in the Template Parameters section.
I wanted it to be prominent during the review. I'm planning to eliminate the note and maybe the template parameter. Do you think it's necessary to use a template here?
_________________________________________________________________ compression.html _______________________________ Acknowlegments
Seward for making their excellent librarys available..." ^^^^^^^^ libraries
at least I didn't say libaries. ;-)
_________________________________________________________________ file_based_resources.html _______________________________ Overview
"...access to memory-mapped files on Windows an POSIX." ^^ and _______________________________ Acknowledgments
"The memory-mapped file resources are based on work of Craig..." ^^ on the
Hmmm. Omitting the 'the' seemed more natural to me. To check common usage, I tried the following Google query: [ "on work of" indebted ] and got this: http://tinyurl.com/4q82c. So I guess you're right. ;-)
_________________________________________________________________ file.html
______ basic_file_source::basic_file_source
"Constructs...std::basic_filebuf buf opened as follows:" ^^^ but
I think 'buf' is residue from basic_filebuf and should be stricken.
_________________________________________________________________ rationale.html
______________ Generic Design
"2. The library can be easily extended to handle..." ^^^^^^^^^^^^^^^^^^^^^^ can be extended easily (avoids split infinitive)
Where's the split infinitive? (Don't make me quote Fowler again ;-)
______________ Chain Interface
"If there is a need for it, it can easily be restored." ^^^^^^^^^^^^^^^^^^^^^^ can be restored easily (avoids split infinitive)
Ditto.
_______________________________ Planned Changes
"2. The concept Buffered will be renamed MultiCharacter, since it is more descriptive and Buffered would be better used elsewhere."
I don't understand this point. "MultiCharacter" does not convey anything useful. Even an unbuffered filter processes multiple characters.
Currently, 1. All filters, even those which do not model Buffered, use a buffer by default, for efficiency. 2. Bufered filters can be used without buffering, by specifying a buffer size of 0. The real difference between Buffered filters and non-Buffered filters is that the member functions of Buffered filters accept a *buffer* of characters: read(Source&, char*, streamsize) write(Sink&, const char*, streamsize) This is why I think MultiCharacter is more descriptive. I'd like to use Buffered for those resources which have their own internal buffers so that the library won't automatically add an unnecessary layer of buffering.
"4. ...This is necessary to accommodate i/o models just as..." ^^^^ such?
Yep.
_________________________________________________________________ adapt.html
______ Return Value
"Returns a Resource...delimited by first and last." ^^^^^^^^^^^^^^^^^^^^^^^^^^^ It is probably well understood by all readers, but it might be better to express this more formally.
Suggestions?
_________________________________________________________________ back_inserter.html
Broken link to
<http://home.comcast.net/~jturkanis/iostreams/libs/io/doc/functions/classes/b... _inserter.html#synopsis> I can't find it.
_________________________________________________________________ close.html _______________________________ Description
"This gives filters and resources an opportunity to free resources or reset their states in preparation for a fresh..." ^^^^^^ state
Are you sure? "The
_______________________________ When is close invoked?
It is premature to discuss the invocations of close() without showing its synopsis. You should put the synopsis just after the Description section. ______________ streambuf_facade and stream_facade
This section is a bit confusing. First you say that r.close() calls boost::io::close(r, std::ios_base::in) followed by boost::io::close(r, std::ios_base::out). Then, you say that if "r" controls a single sequence, those calls wind up, in effect at least, calling r.close().
This is wrong. Call the streambuf_facade instance 'sb'. Then calling sb.close() causes boost::io::close(r, std::ios_base::in) and boost::io::close(r, std::ios_base::out) to be called. These, in turn call r.close().
______________ filtering_streambuf and filtering_stream
You used the term "complete" here. I recall it being defined elsewhere, but it would be useful if you had a single point of definition to which you could link whenever you mention it.
The sequence of close() calls never mentions buf1. Is there a mistake? If not, some clarification is in order.
No, that's correct. buf1 only needs to be flushed using sync().
"...each element receives two closure notifications..." ^^^^^^^^^^^^^^^^^^^^^
I agree that this should be rephrased to eliminate 'notifications', but I don't think it's correct to say that an element receives calls.
_________________________________________________________________ copy.html _______________________________ Description
"The function template copy reads data from a given model of Source - or from a standard input stream or stream buffer - and writes it to a given model of Sink - or from a standard output stream or stream buffer - until the end of stream is reached."
This sentence is confusing due to the number of hypenated phrases. If I've got it right, it should be written like this:
As I explained yesterday, I restored the status of standard streams and stream buffers as full-fledged models of Resource, so the hypenated phrases can be removed.
"The size of the temporary buffer used may be supplied..." ^^^^^^^^^^^^^^^^ temporary buffer between the input and output streams
How about: The size of the temporary buffer used for data transfer may be supplied. ?
_________________________________________________________________ get.html _______________________________ Reference ______________ Description
On many pages, "Description" is the topmost section. Here, it's a subsection of "Reference." Why? This section duplicates the information provided in the Overview, so one of them is superfluous. (This applies to many pages.)
Yes, I need to get this straight.
______________ Return Type
If the return type is known to be "typename std::char_traits<typename char_type<Source>::type>::int_type" why not just show that as the return type in the synopsis?
Because it's ugly :-) How about [see below]. BTW, I'm considering changing the return type to allow it to represent three types of cases: 1. a character was extracted 2. EOF was reached 3. No characters are surrently available -- try back later. I think this is one of the most important interface issues -- what is your opinion?
_________________________________________________________________ read.html _______________________________ Overview
"The first overload.... The second overload...."
No overloads have yet been shown, so this is premature. Move these sentences to Reference|Semantics.
I see your point, but I'd like this information up front. How about 'one overload ... the other overload'?
______________ Template Parameters
"T - For the first overload, a model of Source or a standard input stream or stream buffer type. For the second overload, a model of InputFilter."
The fact that you must describe the parameter this way is a clear indication that you need to use better parameter names. How about:
template<typename Resource> std::streamsize read(Resource& t, ...);
template<typename InputFilter, typename Source> std::streamsize read(InputFilter& t, Source& src, ...);
Since the definition of "Source" refers to T's character type, you'll have to make reference to Resource's or InputFilter's character type. With these distinctions in place, the subsequent Semantics sections won't need to replicate the declarations.
I think renaming the parameters is a good idea, but that recapping the synopsis is also helpful.
_________________________________________________________________ reverse.html _______________________________ Description
"The function template reverse takes a filter and returns a new filter which performs the same filtering operation as the original filter but which is an OutputFilter if the orginal filter was an InputFilter and and InputFilter if the orginal filter was an OutputFilter. Roughly speaking, it reverses the direction of a given filter."
The ideas here are conveyed in the wrong order
in what way?
and why, "roughly speaking?"
Because I haven't formally defined the 'direction' of a filter.
The function template reverse reverses the filtering direction of a given filter. It turns an InputFilter into an OutputFilter and an OutputFilter into an InputFilter.
You mention memory overhead that can be a side effect of using reverse(). Are there performance issues as well?
As long as there's plenty of memory around, performance should be better than if reverse weren't implemented in terms of one_step_filter.
Also, shouldn't that paragraph be set off with "Note:" or "Warning:"?
I like "Warning".
You mention that using reverse may not be suitable for some data streams, but you need to give more guidance regarding why one would want to use reverse().
Okay. The reason is that you may have an implementation of a filtering algorithm only as an output filter, but want to use the same algorithm for an input filter. As I mentioned before, some algorithms are much easier to implement one way than the other.
_________________________________________________________________ seek.html
______________ Function Parameters
"way - ..."
You refer to std::ios_base::beg twice. The latter should be std::ios_base::end. Also, "way" would be better named "dir" or "direction," don't you think?
The standard uses 'off', 'way', 'which'.
_________________________________________________________________ io_traits.html _______________________________ Overview
"The header <boost/io/categories.hpp> contains category tags for classifying models of the various filter and resource concepts." ^^^^^^ ^^^^^^^^ Filter Resource
Here's a good place where lowercase seems clearly appropriate. Is Filter a Filter concept?
"The header <boost/io/io_traits.hpp> contains the definitions of the metafunctions char_type and category, used to associate two fundamental types with each model of one the filter or resource concepts:"
I'm not quite sure how the end of that sentence should be phrased, but it's confusing as is. Perhaps this conveys the right information clearly:
...used to determine the type of characters processed by, and the category, or capability description, of Filters and Resources.
How about: The header <boost/io/io_traits.hpp> contains the definitions of the metafunctions char_type and category, which associate with each model of Filter or Resource its character type and category. ?
______________ Character Type
This section seems out of place. It should be titled, "char_type," but you haven't introduced that template yet. I suggest that this descriptive text be moved to the Class Template char_type|Description section. ______________ Category
This section, too, is out of place. I suggest moving the content to Class Template category|Description.
Thanks. This page was a mess, but I wan't sure what to do with it.
_______________________________ io_traits::type
Where did this come from? I don't understand how io_traits::type is related to char_type (without looking at the code, of course).
This must be a holdover from the days when io_traits was a 'blob' representing two metafunctions. Please point out where you see this, so I can eliminate it.
_______________________________ Category Tags
"...simply define a struct extending the existing tags." ^^^^^^^^^ (multiply) derived from
"simply define a struct deriving from each of the existing tags using multiple inheritance" ?
The table doesn't which tags are mutually exclusive. Is that exclusivity enforced? For example, a type cannot be both a Filter and a Resource, so only one of filter_tag and resource_tag may be used to create a legitimate category. Is that enforced?
Categories are validated to a limited extent.
Is a category tag validated in any way? That is, what if a developer specifies neither filter_tag nor resource_tag. Is the category tag still valid, will it produce a meaningful error, or will it wreak havoc?
You'll definitely get compiler errors, perhaps along the lines of 'filter_wrapper_impl<any_tag> has no member function read'. I guess I should have a valid_category metafunction and use BOOST_MPL_ASSERT(valid_category<category>) frequently.
_________________________________________________________________ mode.html _______________________________ Description
"Metafunction returning a mode tag corresponding the the mode of a model of one of the filter or resource concepts or of a standard stream or stream buffer."
mode is a metafunction that determines the mode type that indicates the mode of the Filter or Resource. The mode can indicate whether the type is usable for input, output, or both, and whether and how the type supports seeking.
Good -- but how about mode is a metafunction returning a tag structure indicating the mode of a Filter or Resource. The mode indicates whether the Filter or Resource is usable for input, output, or both, and the type of seeking it supports, if any.
_________________________________________________________________ filtering_stream.html
_______________________________ Reference
"Tr - The traits type"
You should mention that std::char_traits<Ch> is the default for this parameter (it's in the synopsis, of course, but it bears repeating).
Okay
You should also document which features of the traits class you need so folks that want to write their own can. (You might capture this information in one place to reference from multiple places.)
Surprisingly, perhaps, the traits type is really not needed. I'm assuming that anyone who writes a new character type will also specialize std::char_traits (I should say this). The traits template parameter is provided just so people can construct streams and stream buffers with an alternate traits type, in case the alternate traits type is required by some other API. It won't affect the behavior of the stream or stream buffer. For example, suppose there is an existing library that has a function of the form void f(std::basic_istream<japan_char, japan_traits>&); If I didn't provide the traits parameter, instances of filtering_stream could never be passed to this function.
______________ filtering_stream::filtering_stream (#4)
The description leaves me wanting more. IIUC, this creates a complete filtering_stream with no Filters that uses an iterator based Resource. For input, that would mean that only the characters in the range [first, last) are read. For output, that would mean that characters are written through *first++ until first == last. If I've gotten that right, you need to say something like that so no one else must deduce the same information. Also, this filtering_stream would be complete, right?
Yes.
There's no parameter list introductory statement like, "The parameters have the following meaning." In fact, you've been inconsistent in this on many pages. However, adding a "Function Parameters" heading should be sufficient in and of itself.
______________ filtering_stream::push (#1)
"t - An instance of T"
Can you describe this better?
Given the detailed description of T, what more is needed?
______________ filtering_stream::push (#2)
The signature of this function is identical to the previous push overload. Thus, you should merge the two in the synopsis and when documenting them.
The first takes a const reference, the second a non-const reference.
______________ filtering_stream::reset
"Clears the underlying chain. If the chain is initially complete, causes"
If I understand correctly, that should be worded more like this:
If the chain was complete before calling reset, it closes and releases all Filters in the underlying chain. In either case, it closes and releases the Resource, if any.
The chain can't contain a Resource unless it's complete. So it should be Clears the underlying chain. If the chain was complete before calling reset, causes each filter and resource in the chain to be closed using the function close. Thanks again. Best Regards, Jonathan