"Thorsten Ottosen" <nesotto@cs.auc.dk> wrote in message news:ch86rj$rqb$1@sea.gmane.org...
Hi Jonathan,
Thanks for the review!
here are some general comments.
1. Your documentation is clearŽ, plenty and very well organized. It must have been a huge work!
It was exhausting!
2. How is boundcheking applied in this example:
struct vector_sink : public sink { vector_sink(vector<char>& vec) : vec(vec) { } void write(const char* s, std::streamsize n) { vec.insert(vec.end(), s, s + n); }
I mean, could't n be too big?
This is supposed to extend the vector. Are you worried that n + vec.size() will exceed std::vector::max_size()?
3. The example
filtering_streambuf<input> in; in.push(alphabetic_input_filter()); in.push(cin);
mentions something about the source comming last. Maybe it would be more explicit to say
in.push_filter(alphabetic_input_filter()); in.push_source(cin);
assuming there is a stakc of both types. If there is only one source, then maybe in.attach_source( cin );
There is just one stack. It consists of zero or more filters with an optional resource at the end. See the diagrams here: http://tinyurl.com/5v2ak. Resources represent the ultimate data source/sink, while filters modify data as it passes through. So it wouldn't make sense to have a resource in the middle or a filter at the end. One of the original motivations for introducing the i/o categories (they turned out to be useful for a lot of other things) was to avoid having separate functions push_filter and push_resource. I consider it a major simplification of the interface.
4. in the example
int c; char* first = s; char* last = s + n; while ( first != last && (c = boost::io::get(src)) != EOF && isalpha(c) ) { *first++ = c; }
how can get(src) == EOF, but first != last? Is this "double" checking necessary? Could it be avoided with another design?
This function, from the tutorial, reads characters from an arbitrary Source src into a bufer s of size n. It's possible that the end of the character sequence represented by src could be reached before the buffer is full.
5 In the example, void write(Sink& snk, const char* s, streamsize n) { while (n-- > 0) boost::io::put(snk, toupper(*s++)); }
maybe a comment should say that streamsize is signed.
Good point. In fact, I shouldn't assume people know what streamsize and streamoff are. I should have an introduction to the basic types from <ios> somewhere.
6. tab_expanding_output_filter.hpp maybe we should ask JK about using the new boost license
Good idea. (Maybe I can talk him into reviewing the library.)
7. in presidential_filter_example.cpp (std::streamsize) sub.size() );
should this not be boost::numeric_cast<std::streamsize>( sub.size() ); ? (there is probably more)
I was lazy here ;-)
8. in usenet_input_filter.hpp
dictionary_["AFAIK"] = "as far as I know"; dictionary_["HAND"] = "have a nice day";
maybe boost.assign could make the example even coolor?
Of course it would be cooler! But I think it's good to introduce just one new thing at a time.
9. the old discussion of
typedef typename char_type<T>::type char_type;
I admit this is ugly. That's why I tend to use BOOST_IO_CHAR_TYPE(T) :-)
vs
typedef typename io_char<T>::type char_type;
pops up. If there ever is a vote, I would vote for the last version.
Okay.
Also, io_category<T>::type seems better to avoid confusion with other category traits.
I agree that the nested type should be called io_category, in case something is both an iterator and a resource. But isn't the namespace io good enough to disambiguate the metafunction? Wait ... I was the one arguing for calling the metafunction by the exact same name as the nested type. ... I'll have to think about it some more.
10. In example like
filtering_istream in(adapt(s.begin(), s.end())); filtering_istream in(s.begin(), s.end());
it seems that you could remove the iterator pair version and provide a ForwardReadableRange version
The problem is there's no way to distinguish at compile time between an arbitrary use-defined filter or resource and an arbitrary ForwardReadableRange (unless you're suggesting I do a lot of member-detection). As I explain in Rationale-->Planned Changes-->Item 3 (see http://tinyurl.com/6rtkz), I'm planning to treate boost::iterator_range as (almost) a model of Resource, eliminating the need for adapt. (I'm having second thoughts about one part of that paragraph, though. Identifying output iterators using is_incrementable will misclassify Larry Evans's indentation filters.)
11. What is the difference between the two examples in "III. Appending to an STL Sequence" besides one is much more clumsy?
I assume you mean the first is more clumsy. Unfortunately, it's also more efficient. It uses a plain streambuf_facade, while the second example uses a filtering_ostream, which delegates to a (length one) chain of streambuf_facades. The filtering infrastructure has some overhead, so unless I'm actually going to add filters to the chain I'd prefer the first example.
here are somereview comments: ======================
What is your evaluation of the design? ---------------------------- It seems very well-though out and crisp.
Thanks.
What is your evaluation of the implementation? ----------------------------- haven't looked.
What is your evaluation of the documentation? ----------------------------- puts most other docs to shame.
What is your evaluation of the potential usefulness of the library? ------------------------------ high. I never fully had the time to get into the iostreams framework; it simply seemed to compplicated for a weekend study. I'm very pleased to see yet another library that seems easy to use, yet very
Thanks! powerful. This library shows why we should love C++! ;-)
Do you think the library should be accepted as a Boost library? ===========================================
yes.
Great.
here are some directions I would like to see (but don't require)
1. Use of boost,range instead of iterator pairs
2. possible wierd questions like mine above and others that creep up during
This is planned. the review might form a basis for a FAQ The review has brought to light a number of insufficiencies with the docs. A lot of stuff that seemed obvious to me just isn't. So I'll definitely add a FAQ, but I also need to write a more complete introduction.
3. I hope differences and commonalites with Daryle's MoreIo stuff can be solved; perhaps by the two authors in combination.
I've made some overtures to Daryle w.r.t combining the libraries. I've learned from his response to my review of More IO that he believes the stream buffers in his library should be written from scratch to minimize included code. I hope we can work something out.
Great work!
Thanks again.
Thorsten
Jonathan