"Rob Stewart" <stewart@sig.com> wrote in message news:200409131343.i8DDhOa15411@lawrencewelk.systems.susq.com...
From: "Jeff Garland" <jeff@crystalclearsoftware.com>
We are extending the IOStreams review for one week until 9/11/04. While we have had many good reviews, the library is quite large and we would like to see addtional discussion and review.
I apologize for not getting my review in by 9/11, but I simply couldn't finish it by then. Even now, I cut things short. Nevertheless, here is my review.
There are many comments below, but I'll provide the executive summary first.
Thanks for your very detailed comments! I really appreciate the enormous amount of work you have obviously done examining the library interface and documentation. Responding to your comments is also an enormous amount of work ;-) Here is my reply to the points you make in about the first half of your post. I'll try to reply to the rest tomorrow.
_________________________________________________________________ Should the library be accepted into Boost?
Absent Daryle Walker's strong comments against this library, I'd have said, "Absolutely." Given those comments, I must be more guarded.
The MoreIO library did less to simplify creating and managing streams and the cognitive complexity thereof, and it does not provide the filtering that the Iostreams library offers. Still, MoreIO offers some facilities and an approach that leads to questions regarding whether these two are competing or complementary libraries. While more investigation regarding the strengths and synergies of the two libraries seems warranted, it may be best to accept both and sort out the matter later.
There is no overlap between the manipulators from More IO and the present library. I voted to accept all the manipulators (partly as a result of your comments) so I don't see any problems there. streambuf_wrapping doesn't really overlap with the present library either. I voted to reject because I thought it didn't simplify writing streams enough to justify including it. If it is accepted, however, I don't see any real conflict. So the real question is: if one or more of null_buf, pointerbuf array_buf and value_buf (I may be spelling these wrong) is accepted, would it be advantageous to reimplement it using the present library. There are two issues: 1. Performance Reimplementing pointerbuf and array_buf using the present library should produce exactly the same runtime performce, since resources which present their character sequences as in-memory arrays are specially optimized (see direct_streambuf.fppp at http://tinyurl.com/3qrsx). Reimplementing null_buf and value_buf should improve runtime performance, since the implementations from More IO are not buffered. The reimplemented versions of null_buf and value_buf will have somewhat larger generated code size. Perhaps this would also be true for pointerbuf and array_buf. Using the reimplemented versions will also cause more source code to be included during compilation. This is because Daryle's implementations are self contained while and mine depend on parts of type traits and mpl as well as a certain amount of internal infrastructure. Daryle clearly feels this is an important issue; I think the amount of code included is in keeping with other boost libraries. 2. Interface. Streams and stream buffers generated using the present library have a simple open/is_open/close interface similar to std::fstream. I think including the streams and stream buffers from More IO as is, rather than giving them a compatible interface, will be confusing to users.
_______________________________ Design Evaluation
The library is well conceived and executed. The design seems sound. There are still questions regarding the overlap and differences between this library and MoreIO. There are fundamental, philosophical differences between the libraries.
Could you elaborate?
Use cases should be conceived and compared to see how the libraries can handle each. Only then will we know whether they are complementary or competing and, if the latter is the case, which proves superior according to various criteria including ease of use and performance.
See the above discussion on performance.
It may well be that the stream[buf] parts of MoreIO should simply be added to, even integrated with, this library.
I prefer integration.
_______________________________ Documentation Evaluation
The documentation is astoundingly thorough and quite clear overall.
Thanks!
Many have already made suggestions for improvement, but those can only be seen as making good better.
I'm glad to hear you say that. There were so many comments on the inadequacey of the docs that I was starting to think they were simply awful.
I have included lengthy comments on the documentation, but I could not even visit every page!
_________________________________________________________________ Library Name
I agree with the sentiment of other reviewers in that the library is misnamed. Since this library provides filtering, complete with chaining, as well as to make it easy to create streambufs and streams, the current name fails to capture all of these features.
I agree, but couldn't think of anything better.
Since the Standard Library provides streambufs, streams, manipulators, etc., and yet it is called the IOStreams Library, perhaps there's nothing wrong with simply calling this library Boost.IOStreams. (I do think this library should use the same capitalization: "IOStreams" not "Iostreams.")
Steve Teal's "C++ IOStreams Handbook" and Langer and Kreft's "Standard C++ IOStreams and Locales" use the spelling 'IOStreams'. The C++ standard, however.uses 'Iostreams'. (See 27.1 [lib.iostreams.requirements]). I think 'IOStreams' looks a bit funny, but I'm happy to use it if that's what people like.
_________________________________________________________________ Design Comments _______________________________ Miscellaneous
Wouldn't the InoutResource concept be better named "BidirectionalResource," though that is longer? (Same for InoutFilter.)
Bidirectional was the name I first gave it (http://tinyurl.com/4gskg). I changed it because the filters and resources in question manage two separate sequences, whereas bidirectional iterators represent a single sequence. I chose 'inout' because such components act like an input component glued to an output component. I don't like the name much, and would happily revert to bidirectional, I people don't feel that it's misleading.
"Peekable" does not imply being able to put back a character.
I know. The natural choice would have been Putbackable, which sounds terrible. I choice Peekable because if you can put back a character than you can peek ahead in the stream by reading a character and then putting it back.
Wouldn't "Undoable" be a better name for this optional behavior?
It sounds a bit to general. For instance, an Undoable Sink might allow characters already written to be cancelled.
The normal template parameter abbreviation for a character type is "Char." I don't think abbreviating it to "Ch" is helpful.
The standard uses charT; so do Langer and Kreft. The Dinkumware docs use Elem. I don't recall seeing Char, but I'm not against it.
There's no information provided on the performance of this library. How much overhead does it add compared to custom solutions?
Performance comparable to hand-written components is listed as one of the main design goals. (Rationale-->Design Goals-->Item 3, at http://tinyurl.com/3z9ou). The footnote to that item (http://tinyurl.com/4th3v) mentions some performace comparisons I performed. To summarize, I found that for output, stream_facade<file_descriptor_sink> was slightly faster than the Dinkumware std::ofstream (but it does no synchonization.) Since the overhead of filesystem access might be expected to dominate in that comparison, I also tested a custom ostringstream against the Dinkumware component, and found identical performance. It would have been natural to test input and random access as well. I didn't do this because I wasn't convinced that the tests I was running (from <libs/io/test/detail/verification.hpp>) had any relation to typical use cases. (BTW, I expect random-access performance won't be so good right now, since I haven't optimized streambuf_facade::seekoff for short, frequent seeks. This can be done if necessary.) I didn't test filtering performance because at the time I didn't have anything to compare it with except for James Kanze's implementation, which uses pre-standard streams. Now that I have JC van Winkel's filtering implementation and Robert Ramey's Dataflow Iterators, I can run some head-to-head tests. I have spent a lot of effort on optimization, and expect my implementation to compare favorably.
For example, how much memory and runtime overhead is there wrt MoreIO's mechanisms?
There should be slightly more memory overhead with the reimplementation of null_buf and value_buf using my library, since it provides buffering by default.
How much overhead is there wrt a custom filtering solution? These are answers users of the library need to judge whether it is sufficient or must they choose another approach.
See above.
The filter chain ordering for input versus output is awkward. It should always be from the beginning to end; let the library reverse the order.
I'm surprised nobody mentioned this earlier. Having the resource at the end dramatically simplifies the interface. It allows an instance filtering_stream of filtering streambuf to be considered 'open' as soon as a resource is pushed, and 'closed' as soon as it is popped. This allows a simple stack-like interface. The alternative would be (i) to add open/close functions (which are part of the interface of streambuf_facade, but would currently be redundant for filtering streams, OR (ii) to have a first-time switch which automatically 'opens' the filtering stream as soon as the first i/o is performed. In addition, allowing filters to be pushed after a resource would give many new users the impression that they can add filters *after* i/o is in progress. As has been discussed during the review, this is not currently supported; support can be added in limited circumstances, but not generally. Consider: filtering_ostream out; out.push(file_sink("log")); out.push(base_64_encoder()); out << "hello world!\n"; // stream is implicity 'open' out.push(zlib_compressor()); // error!
_______________________________ boost::io::converter
"converter" isn't well-named.
How about code_converter?
It only widens characters for the resources it converts, so it should be called something else.
converter is basically an easy to use wrapper for codecvt, which is the standard library's component for code conversion. So 'code_coverter' (or converter, for short) seems appropriate. George Garner suggested that reverse code conversion be supported too, and I am inclined to agree, though it is much less useful.
So that begs the new name. I thought of "widener," but while it's correct English, it's also odd sounding, at least to my ear. "character_widener," would be a little more explicit.
This is a bad choice, since 'widening' is performed by a ctype, not a codecvt.
Perhaps "expander" or "character_expander" would work.
Same as widener -- to me it implies a character-by-character transformation.
Can converter be used with a wide character resource? Imagine a generic "widen" which ensures that the resulting streambuf or stream uses wide characters.
In theory, yes. If you supply a locale containing a codecvt<wchar_t, wchar_t>, then you could use converter<wfile_source> to "widen" a wide-character stream. However, you can't depend on an implementation providing specializations codecvt<wchar_t, wchar_t> or even codecvt<char, wchar_t>. I'm inclined to parameterize converter by a codevt type, as suggested by George Garner: template< typename Resource, typename Codecvt = fetch_from_global_locale > If Codecvt::extern_type does not match the character type of Resource, but Codecvt::inter_type does, the codecvt will be automatically 'reversed'.
convert's "Int" template parameter is poorly named. "Int" is, of course, a common abbreviation for "integer" in C++. I suggest using "Intern" or even "Internal." The name of "IntTr" should follow suit.
True. I think I was trying to fit within the 80 character line limit, without too many line breaks.
_______________________________ basic_newline_filter
I realize that this is only an example, but there is no apparent protection from misconfiguring the constructor flags. Grouping the related options into enumerated types and taking separate parameters for each group would make it safer to use. (You can overload the bitwise OR operator to permit combining them conveniently.) That is, since print_CR, print_LF, and print_CRLF are mutually exclusive, they should be part of an enumerated type that does not permit bitwise OR'ing. Since the posix, mac, and windows values are meant to supplant all of the other values, they should be part of their own enumerated type and should be used in a separate constructor. The remaining options can be part of a third enumerated type, with bitwise OR support, that constitutes the second argument to the existing constructor.
I was trying to keep things simple. Specifying more than one of print_CR, print_LF and print_CRLF yeilds a runtime error. Even if print_xxx had it's own enumeration type, it would still be possible to specify illegal values.
The ignore_CR and ignore_LF values are poorly named. I suggest ignore_extra_CRs and ignore_extra_LFs.
Sounds reasonable.
The print_CR, print_CRLF, and print_LF values would be better if named write_CR, write_CRLF, and write_LF, since no printing is occuring, at least in general.
How about output_xxx instead of print_xxx?
_________________________________________________________________ boost::io::reverse
Isn't the direction of a filter independent of the filtering?
Yes, in theory, put in practice some filters are much easier to express as an output filter than an input filter, or vice versa.
That is, why isn't a filter's interface based upon these semantics:
char_type filter(char_type ch); streamsize filter(char const * input, streamsize n, char const * output); Then, the source and destination of the data is under the control of the filter() function and means that whether the filtering is used on input or output is an orthogonal decision. This interface should be every bit as efficient as what you've specified, but makes filters more straightforward and flexible.
This is what symmetric filter does (http://tinyurl.com/3lz82). I'm planning to optimize the case where a symmetric filter is added to a filter chain, so that symmetric filters should be ruthlessly efficient. The problem with making symmetric filters the default (or exclsuive) filter concept is that it's unexpectedly difficult to implement them correctly.
_________________________________________________________________ Documentation Comments
Since I agree with most of your suggestions, I'll only comment on those where I disagree or where further explanation is needed.
In case it makes a difference, I was reading the documentation at http://home.comcast.net/~jturkanis/iostreams/libs/io/doc/ on 4 SEP 2004 rather than downloaded documentation.
That's fine.
_______________________________ General
"boiler plate" is one word; I'm guessing it will appear in various places throughout the documentation, so I'm calling it out at the top.
Thanks for pointing that out. (I'd guess it went through the typical transformation: boiler plate --> boiler-plate --> boilerplate.)
When formatting source code may I suggest not indenting twice for braces? Doing so pushes code too far right and reduces readability due to long lines. (This occurs on tutorial.html and read.html, for example.)
Do you mean: struct tolower_filter : public buffered_input_filter { template<typename Source> streamsize read(Source& src, char* s, streamsize n) { streamsize result = boost::io::read(src, s, n); for (streamsize z = 0; z < result; ++z) s[z] = tolower(s[z]); return result; } }; Should be struct tolower_filter : public buffered_input_filter { template<typename Source> streamsize read(Source& src, char* s, streamsize n) { streamsize result = boost::io::read(src, s, n); for (streamsize z = 0; z < result; ++z) s[z] = tolower(s[z]); return result; } }; ? Sounds like an improvement.
When referring to concepts, always capitalize them. Thus, "filter" becomes "Filter" in most uses, for example.
I realize I don't follow a consistent convention, but I've tried to distinguish beetwen concepts, models of concepts, and instances of models of concepts. Obviously I haven't done a very good job, but I'm not sure using the capitalized version everywhere is the right solution. (Maybe it is)
In many places you mention that something works with a Source or a "standard input stream or a standard stream buffer" or similar. Wouldn't it be better to clearly define once that std::basic_istream models Source, std::basic_ostream models Sink, std::basic_iostream and std::basic_streambuf model both?
This is just because I didn't update the docs well enough after the most recent rewrite. FWIW, 1. Originally, standard streams and stream buffers were not models of Resource. 2. Then I went out of my way -- by making the concepts 'external' in Thorsten Ottosen's sense -- to make streams and stream buffers full-fledged models of Resource. 3. Then, for exception safety reasons, I decided that Resources must be CopyConstructible, so that streams and stream buffers would simply be 'treated as' Resources. 4. Finally, I dropped the CopyConstructible requirement but neglected to update the docs everywhere.
_________________________________________________________________ menu.html _______________________________ Contents
This is confusing. Apparently, the leading "+" means a different page, whereas entries without the "+" are on the current page, but there is no legend explaining that.
'+' means that the entry is expandible by clicking. '-' means the entry is collapsable. See http://tinyurl.com/65234.
Why do some pages, like tutorial.html have a mini-TOC at the top, whereas others, like home.html, which may be the only unique one, get their sections listed in the Contents frame?
When I added the tree control, I forgot to get rid of all the local TOC's.
There's nothing wrong with what you've done, but why not use the phrase, "standard streambuf" instead of "standard stream buffer?" It's shorter and in keeping with what anyone with some familiarity of Standard IOStreams would expect.
'Stream buffer' is used by the standard and by Langer and Kreft, presumably because streambuf is the specialization of basic_streambuf for the type char.
_______________________________ Footnotes
I don't see any text that references the footnotes at the bottom of the page, so their presence is confusing.
I forgot to remove them when I moved Tutorial to its own page.
(Further reading reveals that these footnotes appear at the bottom of every page,
I hope not!
regardless of whether they are referenced.
_________________________________________________________________ tutorial.html
" Both streambuf_facade and stream_facade have constructors and overloads of open which forward their arguments to filter or ^^^^^^^^^ to the filter
resource constructor. Therefore, the first example could have ^^^^^ previous
I see a grammatical problem here -- "forward their arguments to filter or resource constructor" -- but I don't understand what changes you are suggesting.
"The function boost::io::get reads a character from an arbitrary Source; it is provided by the Iostreams Library to allow all Sources to be accessed with a single interface."
That's worded awkwardly. I suggest rephrasing like this:
The function boost::io::get reads a character from an arbitrary Source. This is possible because the Iostreams Library provides the same interface for all Sources.
I see your point, but it might be better just to use your first sentence. The second sentence provides more information than needed here.
"The last item added to the chain could be any Source."
In the following example, reproduced here, the order seems backward:
filtering_streambuf<input> in; in.push(alphabetic_input_filter()); in.push(random_source());
If that's the correct order, then you need to explain, even here in the tutorial, why the order is as shown.
I mentioned the rationale, but obviously I need to point out the ordering issues in the tutorial.
"We could also add any number of InputFilters to the chain before adding the Source."
"They could also use the following convenience typedefs:"
I think the first sentence was changed without updating the second. There are several choices for this type of passage: 1. Use the passive voice everywhere. 2. Use 'we' -- this sounds natural to me because it's used in mathematical papers. 3. Use 'you' 4. Use 'the user' Which should one prefer? Settling on one of the above and using it consistently should resolve many of your (snipped) suggestions/objections below.
_______________________________ Efficient Filtering: Buffered Filters
You've changed from "we" to "one" and it sounds odd. Try this:
Agreed.
In such cases, reading a character from an ordinary InputFilter means calling its get member function, thus incurring the overhead of a (virtual?) function call.
It's not virtual.
(It isn't clear from the tutorial whether read supplants get or augments it. I assumed the former in writing the above.)
You assumed correctly.
_________________________________________________________________ examples.html
Presidential OutputFilter
The political humor of this example is unwarranted in Boost documentation.
I'm not sure I agree. But noted.
Perhaps you could rename it to "Texas OutputFilter" or "Southern U.S. OutputFilter."
I don't think this would help. Texans don't typically say 'subliminable,' for instance.
There is no description of what the filter actually does in this section.
I'm thinking each example should have its own page, with a detailed description of what the filter does, a guided tour of the source code, and an explanation of the example output.
_______________________________ Regex OutputFilter
"This example uses a regex_filter to remove C and C++-style comments from a code excerpt. Although somewhat more ^^^^^^^^^^^^^^^^^^^ omit
sophisticated than the Uncommenting InputFilter, above, the ^^^
What's the problem here?
_______________________________ Overview
"...an InputFilter, which filters input read from a Source, and an OutputFilter, which filters output written to a Sink."
I suggest a little rephrasing to make this read better and fill in a couple of gaps:
an InputFilter, which filters input read from a Source or another InputFilter, and an OutputFilter, which filters output read from a Source or another OutputFilter, before it is written to a Sink.
But that would be false. In order to chain filters, the library has to wrap filters with adapters so that they become resources.
"SeekableResource" refers to a read/write "head," but few, if any, Resources for which this library will be used are physical devices with read/write heads. I suggest using "location," "position," or "offset" in lieu of "head" here and in other parts of the documentation.
How about 'stream position indicator'?
_______________________________ Filter Concepts
InputFilter, OutputFilter, InoutFilter, and SeekableFilter refer to a "stream buffer." Shouldn't they refer to Sources and Filters instead?
Right. Until the latest revision, filters had to communicate with stream buffers.
"Notifications" is unexpected and you refer to a stream. Try this:
Closable: A Filter or Resource which is told to close immediately before a Source or Sink is closed.
How about: Closable: A Filter or Resource with a member function close which is called when the end of a character sequence is reached.
In "Direct," you refer to a socket-like interface, but not everyone will understand what you mean. I suggest using the phrase, "function-based interface."
I see your point, but Direct Resources also have a function based interface.
_________________________________________________________________ modes.html
"Readers new to the Iostreams Library should feel free to ^^^^^^^ ^^^^^^^^^^^^ Users omit concentrate primarly on these two modes."
How would you phrase the whole sentence?
_______________________________ Definitions of the Modes
"* Whether a filter or resource is Closable."
This should be a single line.
Seems to be a mozilla bug. Even doesn't help.
"This is useful to help reduce...different of filter types." ^^ omit
I don't follow.
_______________________________ The Metafunction mode
You need to include an example and the signature of mode so we know how to use it.
This section contains a link to the reference docs for mode: http://tinyurl.com/5oqsy. Wouldn't it suffice to provide an example there?
_______________________________ Reference
"Policy-based stream buffer template with an interface...." ^^^^^^^^ class template
"Policy-based class template deriving from a specialization of basic_streambuf with an interface...." ?
______ Synopsis
Using "T" as the name of a policy class template parameter is confusing. "T" is, of course, commonly used for a data type stored in a container and for similar purposes. The closest thing to "T" in streambuf_facade is the "Ch" nested type in the policy class. Change "T" to a more meaningful name.
I adopted this convention from "C++ Templates", p. 10. FilterOrResource would be more decriptive, but it's too long. What would you suggest?
______ Template Parameters
"T - A model of the filter or resource concepts. Specializations of streambuf_facade with filter types are used internally by the Iostreams Library to construct chains of filters and resources. Users of the library never need to specialize streambuf_facade with a filter type."
Since only the library uses filter types for "T", why not name "T" "Resource" and let the library worry about using filters for the "Resource" parameter? Also, given that only the library does this, omit the last two sentences or move them to an Advanced Topics section.
Okay, except that I've been toying with exposing some of the undocumented stuff. Thanks again -- see you tomorrow ;-) Jonathan