From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-09-03 21:37:20
"Pavel Vozenilek" <pavel_vozenilek_at_[hidden]> wrote in message
> > start of the formal review of the Iostreams
> > library by Jonathan Turkanis.
> I vote to include Iostreams into Boost with both hands.
> It is prime example of professional work.
> Looking back on MoreIo library I recommend:
> - to prefere Iostreams for all overlapping functionality
> - to move the applicable rest from MoreIo into Iostreams
> - naming conventions taken from Iostreams
> - documentation look and feel from Iostreams
I assume you view null_buf, pointerbuf, etc. as overlapping functionality. What
about 'streambuf wrapping'?
> For user it is always better to have one library
> with consistent structure/naming/documentation
> than to have many of them. This is hard earned
> As the Iostreams is large, I didn't yet read reference
> docs and test code. I plan it and will post rest
> of review later.
I look forward to it. Note that the end date for this review is not yet fixed.
> Current review notes are bellow.
> To deal with its complexity I recommend to add many
> more overcommented code snippets into documentation
> and add hyperlinks to relevant examples on every fitting place.
> Pictures, flowcharts and diagrams would help too.
> Some information would look better in tabular form.
> (Details bellow).
Yes; the review so far has made this very clear to me.
> 1. overview docs:
> The first occurence of terms "data source"
> and "sink" should be hyperlinked (it is but only later
> in text). Dtto "filters".
My reason for not hyperlinking the first occurence was my feeling that in the
first two paragraphs I was simply using them as 'common sense' terms, but that
when I used the terms in the section marked 'Concepts' I was formally
introducing them as technical terms defined by the library. Obviously this was
not very clear.
> It may be also useful to put there some diagram. In addition
> link to short code example could be added to the first
> occurence for each term.
Good idea. I've become convinced of the need to add explanatory material right
at the beginning.
> The enumeration of components as 'access to memory-mapped files'
> may look better in <ul>...</ul> list.
> Sentence: "Sources, Sinks and their refinements are called
> The term is overused in programming. Maybe "data resources"
> or "data-module" or so would be better.
I know that 'resource' is a loaded term ;-) but I couldn't think of anything
better. 'Data-module' doesn't seem very descriptive. Unfortunately, the standard
term seems to be "Source/Sink" -- which I find ugly.
> Relation between sins/sources/filters and streams should be
> explained immediatelly. Possibly using picture.
Okay. What do you think of the pictures here: http://tinyurl.com/5v2ak? Too much
detail for the introduction?
> Link to "adapters.html" is broken.
> The links to iterator_facade and Iterators library should
> be local.
I'm planning to make all the boost links local eventually. I wanted them to work
properly for people viewing the docs from the zipped package.
> 2. tutorial docs:
> The first code example:
> s[z] = (char) (rand() * 256 / RAND_MAX);
> I think this causes overflows way too often.
> s[z] = (char) (rand() % 256);
> is more likely intended even if it
> adds small bias.
> In example:
> also stream example should be added,
> just to show how it is written.
I don't understand.
> streambuf_facade and stream_facade should be hyperlinked.
> (I am fan of hyperlinking everything everywhere.)
I tried to be liberal with my hyperlinks. Sometimes I find it annoying when the
same word is hyperlinked several times in a sentence. But you're right, in the
tutorial streambuf_facade and stream_facade definitely need to link the user's
guide or the reference.
> In code example line:
> streambuf_facade<vector_sink> out(v); // error!!
> it should be sais if it is compile time error,
> assertion or whether user needs to check it manually.
> I wonder it is is possible to workaround this limitation
> internally in library by const_casts/whatever nasty.
These are the standard forwarding problem trade offs. The one thing I defintely
want to avoid is allowing const references to bind to temporary stream objects:
This would immediately lead to disaster.
> "... and modifies it before returning it to the user."
> should be
> "... and modifies or eliminates it before returning it to the user."
Isn't elimination a type of modification?
> In example:
> filtering_streambuf<input> in;
> It should be explained what is "input" and where
> does it come from.
You're right. To clarify, input is a tag struct representing the i/o mode
So, filtering_streambuf<input> is a read-only filtering stream, synonymous with
filtering_istream, and filtering_streambuf<output> is a write-only filtering
stream, synonymous with filtering_ostream
> Shouldn't it have more expressive
> name e.g. input_filter_base? Maybe the source/sink too.
I don't follow this.
> It is a bit confusing to see push() of both input_filter
> descendant and source descendant. Should be noted,
> maybe with class diagram.
Others have said this too. Originally I had something like push_filter and
push_resource. The i/o categories made this unnecessary. I consider the
std::stack-like push/pop/size/empty interface to be much more elegant. After
all, there's just one stack. Internally, also, adding a filter is essentially
same as adding a resource.
> All examples use variable names 'in' and 'out'. It may be
> confusing for one just skimming through code snippets.
> Maybe different names (e.g. vector_in) could be used.
> It would be also nice to distinguish variables and types
> systematically. E.g. variables having _ suffix.
That's the convention I use for member variables.
> filters.html link is broken.
> Generally, tutorial may be sprinkled with few more
> overcommented code snippets. They may be hide-able.
> Other docs won't suffer having them too.
except for the menu.
> 2. examples docs:
> Link to regex should be local.
> Link "Uncommenting Input Filter" is broken.
If you mean the link under "Regex OutputFilter", I think it appears broken only
because there's nowhere left to scroll.
> 3. presidential_output_filter.hpp: a nitpick, in code:
> if (word == "subliminable")
> return "subliminal";
> else if (word == "nucular")
> the 'else' parts are not needed.
> 4. docs: all <img> elements should have their width/height set in HTML
For slow connections, I guess?
> 5. concepts docs: all concepts here can be also listen in table,
> sorted and with one line info.
> The individual concepts should be added as sub tree into left panel.
> Dtto compression filters, etc.
> Every page should be accessible from left panel.
I'll try to do this, but I might find that the menu is so big it takes too long
> 6. Installation docs: it should be in moredetail described what is
> BOOST_IO_NO_LIB for and who needs it for what.
> Maybe there should be list of *.cpp files one needs
> to use for zlib/bzip2/..., for those who do not want
> to use autolink.
All the .cpp files are enumerated here: http://tinyurl.com/6klot.
> 7. "Classes by category" docs: each link could have very short
> decription on the left:
> array_resouce (xyz...)
> array_sink (abc...)
> Similarly classes sorted alphabetically, functions, etc.
> It would help when one is trying quickly locate some info.
> 8. All applicable headers should have on top:
> #if (defined _MSC_VER) && (_MSC_VER >= 1200)
> # pragma once
> to keep compilation times for VC and Intel C++ down.
I thought I already made this change.
> 9. Syntax like this was suggested:
> filter_stream out(tee(std::cout) | encode | gzip | file("some file"));
> Maybe support for this could be built up on existing framework.
I think it's really cool. I've already contacted the (co-)author JC van Winkel,
who gave me permission to use it.
> 10. To question:
> > do you prefer the names filter_stream and filter_streambuf to
> > filtering_stream and filtering_streambuf?
> The first. Steve McConnell in his Code Complete 2 quotes
> study recommending identifier length 10-16 chars.
> 11. A collection of useful stream(buffer)s could be added into library,
> so they are handy to users:
> - /dev/nul like stream (like the one from more_io)
> - tee, tee3, ... etc stream
> - random (the one from code snippet)
These are all good. I'm thinking as the collection of provided components grows,
I may need to be more systematic about library organization. So one might have
directories boost/io/filters/compression, etc.
> - repeater (some sequence of characters over and over)
> This one could ideally accept syntax used inn Boost.Assign.
> It would allow to easily create complex but predictable
> test data structures.
Could you elaborate?
> - 'a switcher'. Something what sends data into another stream
> but has method switch_to_other_stream(s). E.g. for circular logs.
Doable, but presents some problems with buffer synchronization. There is
currently no generic sync() function or Synchronizable concept.
> 12. One could dream about ability to use lambda expressions directly
> for filters.
Lambda expressions which define function objects which transform characters one
at a time whould be easy. But I suppose you mean lambda expressions representing
complex filtering operations.
Let's see ....
How about this: instead of place-holders _1, _2, etc. we could have _get, _peek,
_put... The following could be an inline version of Kanze's uncommenting input
if_(_get == '#')
while_(_peek != EOF && _peek != '\n') [ _get ]
> 13. Possible feature of library (I do not know whether it is
> implementable or implementable efficiently/elegantly):
> - lets have chain of streams (e.f. buffer/compress/write file)
> - someone feeding the head of chain may want to execute an action
> somwhere down in the chain (e.g. switching files).
> - now it would require 'someone' to know all details of chain
> and to have access points to its parts (=> high coupling)
Right. That's the status quo.
> It would be nice to be able to 'send a command' down the stream
> and have some part of chain picking it up and executing.
> This way sender would not need to know anything about
> current chain structure.
> If the command wouldn't be recognizable by any part of stream
> chain it would become nop.
> Similarly one can imagine events going upstream.
> If anyone handles them: OK, if not they will be ignored.
> (They are different from exceptions that ignoring them
> is default and harmless.)
Sounds interesting. I believe this would have to rely on runtime rather than
compile-time polymorphism. I'll think about it.
> Examples of downstream commands:
> - switch file (for logging)
> - flush compressor dictionaries
> Examples of upstream events:
> - predetermined file size reached
> - current compression ratio info
> 14. May it be possible to add "synchronize panels" button into
> left panel? If clicked, it would expand and highlight item
> in left panel for currently opened page in right panel.
The [link to this page] button already expands the tree to the current page, if
the tree contains a link to the current page. I guess highlighting would be easy
enough to do. I'm afraid some would find this annoying.
> 15, cloable.html: it should be explained (+ example) what
> 'notification' here means.
How's this: A Closable filter or resource receives notifications -- via the
function <A HREF='...'>boost::io::close</A> --immediately before a containing
stream or stream buffer is closed.
I defintely need an example here.
> 16. The examples section should contain all available examples,
> e.g. compressors. There may be subsection for them.
> Maybe something as:
> + Examples
> + Simple Examples
> + Compressors Examples
> + ....
> It is because many people would not go any futher than here
> and would miss the rest.
Thanks for the detailed examination!
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk