[boost] Re: IOStreams formal review start

From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-09-02 18:51:02

"Thorsten Ottosen" <nesotto_at_[hidden]> wrote in message
news:ch86rj$rqb$1_at_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.

Thanks!

> 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
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

This is planned.

> 2. possible wierd questions like mine above and others that creep up during
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