From: "Jonathan Turkanis" <technews@kangaroologic.com>
"Rob Stewart" <stewart@sig.com> wrote in message:
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)
Those would be distinct, but not informative.
or
streambuf_facade::streambuf_facade (Default) streambuf_facade::streambuf_facade (Forwarding) streambuf_facade::streambuf_facade (From Policy)
would be better in cases like this.
Those would be better. However, rather than rely on headings at all, why not just let the synopsis excerpt divide things?
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.
It is more correct. I was trying to say that if you didn't use the "Constructors" heading, but wanted to keep the streambuf_facade headings, then they really should include the template parameters.
______ The second streambuf_facade::streambuf_facade
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.
As a reference guide, I'd expect consistency on this point so that I could easily spot desired information.
______ streambuf_facade::open
"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.
As you might guess from there only being four carets, it wasn't supposed to be under "there is." Using a fixed width font, the only way you can possibly understand such notation, you'll see that I was suggesting s/such/T/.
______________ 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.
So long as you meant "most" and not "all" that should do pretty well.
_________________________________________________________________ filtering_streams.html
_______________________________ Examples
Do any of these techniques apply to filtering_streambufs?
They all apply to the streambuf variants of the stream templates used.
You apparently have more than one filtering_streams.html and I didn't note the relative URL, so I can't find the section to which I was referring. However, I'm pretty certain I was trying to say that you don't mention filtering_streambufs, only filtering_streams in that context.
_________________________________________________________________ code_conversion.html
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 ^^^^^^^^^^ component automatically inserts a code_converter.
s/component/filter/ since this only applies to filters, right? Otherwise, it's fine.
_________________________________________________________________ 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.
Very nice.
______________ 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 avoided "associate" altogether in my rewording. Did I not capture the right idea? Anyway, "attach" and "detach" are still not quite right because you make a copy, right?
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.
That will be good.
______________ 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
Why do you need both "detach" and "destroy?" Doesn't "destroy" say it all?
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.'
LOL
_______________________________ Exception Safety
"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.
I presume you'll mention that in your revised docs.
_______________________________ 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.
Excellent.
_________________________________________________________________ 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"?
Most text I had read in your documentation, I suspect, was grammatically correct. This stood out, I assume, or I wouldn't have flagged it. However, I don't know whether you were consistent in omitting the article. If so, ignore my comment. If not, choose one convention.
"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.
That works.
______________ 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.
That muddies the issue for me.
_______________________________ Example
"See Examples." ^^^^^^^^
I'm not sure I see the problem. Maybe: See Examples (Regex OutputFilter).
s/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) :-)
Superstition? No, it's just been drilled into my head. You're right of course, that there's nothing wrong with "between" in that sentence.
"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?
I don't personally need it, but I can well imagine that folks would need a wide character version. Perhaps in that case, the line endings aren't really wide character?
_________________________________________________________________ compression.html _______________________________ Acknowlegments
Seward for making their excellent librarys available..." ^^^^^^^^ libraries
at least I didn't say libaries. ;-)
8^)
_________________________________________________________________ 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.
It should be removed. I was probably tired and not thinking as clearly as I should have to try correcting your English!
_________________________________________________________________ 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 ;-)
No split infinitive; probably tired again. I do think the change sounds better.
______________ Chain Interface
"If there is a need for it, it can easily be restored." ^^^^^^^^^^^^^^^^^^^^^^ can be restored easily (avoids split infinitive)
No split infinitive; probably tired again. Either version is fine here.
_______________________________ 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.
I understand what you're trying to say now. I'm not fond of MultiCharacter, but offhand, I haven't a better name.
_________________________________________________________________ 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.
[first, last)
_________________________________________________________________ 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.
There are automated tools in Boost and there will be many readers. It will surface again, if it is still present.
_________________________________________________________________ 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
Each filter and resource has but one state. (This may be a gray area, for all I know.)
______________ 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().
OK, but do give thought to clarifying the text.
______________ filtering_streambuf and filtering_stream
"...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.
I assume "element" refers to a filter or resource, in which case calling a function is quite reasonable. What have I missed?
_________________________________________________________________ copy.html _______________________________ Description
"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.
I still think it is useful to mention the entities sharing the buffer.
_________________________________________________________________ get.html _______________________________ Reference ______________ 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].
That may be, but you're hiding the truth.
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?
Given your discussions on async I/O, this is probably necessary, but I've not spent any time considering the ramifications.
_________________________________________________________________ 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'?
I don't understand why that information needs to be in the overview. If one is for internal use, you don't need to highlight it. Without those two bullets, the overview remains complete IMO.
_________________________________________________________________ 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?
The function template reverse takes a filter and reverses its direction of data flow. It forms an OutputFilter if the original filter is an InputFilter, and vice versa.
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.
That's the sort of information needed in the documentation. Of course, I'm still unconvinced of the efficacy of the current filtering approach, as mentioned previously.
_________________________________________________________________ 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'.
Does that make them good automatically?
_________________________________________________________________ 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?
Hmmm. I suppose you're right. Wouldn it be better phrased as, "...various Filters and Resources?"
"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.
That's fine; be sure to make "character type" and "category" be links.
_______________________________ 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.
http://home.comcast.net/~jturkanis/iostreams/libs/io/doc/io_traits.html, about halfway down.
_______________________________ 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"
Is it always multiple inheritance? That's what I was trying to convey by parenthesizing "multiply." Otherwise, it's fine.
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.
Anything to help the user get things right.
_________________________________________________________________ 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.
Fine.
_________________________________________________________________ filtering_stream.html
_______________________________ Reference
"Tr - The traits type"
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.
<confused> Is the traits type needed or not? If so, what facilities must it provide? </confused>
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.
From this I infer that the traits type must meet the Standard's requirements for char_traits (21.1.1).
______________ filtering_stream::push (#1)
"t - An instance of T"
Can you describe this better?
Given the detailed description of T, what more is needed?
How about this: A Resource, if the filtering_stream is to be complete, or a Filters if the filtering_stream is not to be complete yet.
______________ 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.
While I should have seen that, it is easy to miss and documenting them separately is a waste of space and the reader's time. Put the two synopsis excerpts together and document them as one. If you wish, you can mention that difference between the two, but with them adjacent, the difference should be more apparent.
______________ 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.
Oops. I did switch those around, didn't I? Your rewrite fails to indicate what happens if the chain is not complete. -- Rob Stewart stewart@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;