[boost] Re: Review of Daryle Walker's "More IO" library begins today, August 21, 2004.

From: Jonathan Turkanis (technews_at_[hidden])
Date: 2004-08-26 00:38:54

Here's my review.

Let me state first that I have a certain bias, because I have
submitted an iostreams library (http://tinyurl.com/3m6ur) which
overlaps with the current library somewhat. It consists of a general
framework for constructing streams and stream buffers; all of the
stream buffers included in the currently library can be defined with
just a few lines of code using my library, and I believe that is the
better approach.

----------------------------------------

Summary:

<boost/io/streambuf_wrapping.hpp>: Reject, because it reduces the work
required to produce a stream from a stream buffer only marginally, if
at all.

<boost/io/array_stream.hpp>: Reject, as insufficiently motivated.

<boost/io/pointer_stream.hpp>: Accept (with qualification).

<boost/io/null_stream.hpp>: Accept (with qualification).

<boost/io/value_stream.hpp>: Reject, as insufficiently motivated.

<boost/io/iomanip.hpp>: Accept resetios, skipl and multi_skipl and
reject the others.

----------------------------------------

Basic Review Questions:

- What is your evaluation of the design?

The library seems to be a collection of bits and pieces. This is not
bad in itself, but I'd rather see a more systematic treatment.

- What is your evaluation of the implementation?

I checked the implementation only in a few places, because I couldn't
understand the documentation. Most of what I saw was okay, but there
are several problems pointed out below.

- What is your evaluation of the documentation?

The documentation is poor. It seems to consist only of 'reference
documentation'. It needs an overview, examples and perhaps a tutorial.
The descriptions and the rationales are generally insufficient (more
below).

- What is your evaluation of the potential usefulness of the library?

All the components I voted to accept are moderately useful. I don't
think any fills a burning need.

- Did you try to use the library?  With what compiler?  Did you have
any problems?

I tried to use array_stream and found it wouldn't compile one VC7.1
and como 4.3.3, as I reported.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I spent about two hours looking at the documentation and sources and
writing this review. I also read many of the reviews from last time
around.

- Are you knowledgeable about the problem domain?

I am very knowledgable about stream buffers but have only a basic
knowledge of manipulators.

- Do you think the library should be accepted as a Boost library?

See summary.

----------------------------------------

<boost/io/streambuf_wrapping.hpp>:

I vote to reject, for the following reasons.

I don't accept the criticism that the proper way to use a custom
streambuf is with a plain istream, ostream or iostream; it's nice to
have matching stream classes to go with a custom stream buffer class.

However, if one has written a custom stream buffer, the name
basic_wrapping_xstream is not what most people want for the matching
streams. If the streambuf is called mapped_filebuf, a matching i/o
stream might be named mapped_fstream. This could be solved with
template aliases:

    template<typename Ch>
    using mapped_fstream =
        basic_wrapping_iostream< mapped_filebuf<Ch> >;

Using streambuf_wrapping.hpp and the current language, the way to
achieve this is to derive a stream class from basic_wrapping_iostream<
mapped_filebuf<...> >:

    template<typename Ch>
    class mapped_fstream :
        basic_wrapping_iostream<mapped_filebuf>
    {
        ...
    };

The problem with this is that one has to write new constructors, since
they are not inherited. If one wants to follow the example of the
standard library file and string streams, one should also repeat the
typedefs char_type, traits_type, pos_type, etc. As a result, the above
is scarcely easier than writing mapped_fstream from scratch, since
writing the constructors (and typedefs) is the principle difficulty.

Without template aliases, a macro-based approach would be more useful:

    #define BOOST_IO_DEFINE_WRAPPING_ISTREAM(stream, streambuf, arity)
    #define BOOST_IO_DEFINE_WRAPPING_OSTREAM(stream, streambuf, arity)
    #define BOOST_IO_DEFINE_WRAPPING_IOSTREAM(stream, streambuf,
arity)

    BOOST_IO_DEFINE_WRAPPING_ISTREAM(mapped_fstream, mapped_filebuf,
1)

Also, let me note that with my library, streams and stream buffers
are generated simultaneously using stream_facade and
streambuf_facade, so you will rarely need to write a stream buffer
from scratch and wrap it in a stream. E.g.

    struct tcp_resource { ... };
    typedef streambuf_facade<tcp_resource> tcp_streambuf;
    typedef stream_facade<tcp_resource> tcp_stream;

----------------------------------------

<boost/io/array_stream.hpp>:

I vote to reject, for the following reasons.

1. std::stringstream already writes to an internally managed character
array, but it suffers from the following limitations (both resaonable
in context)

  a. You can't define a stringstream to access a region of memory
specified in advance
  b. when you finish performing i/o, you can't get direct access to
the underlying array.

array_stream addresses b but not a, and doesn't address b very well,
since the lifetime of the array is included in the lifetime of the
stream. I believe the pointer streams are better in every respect than
the array streams.

2. basic_array_streambuf should be implemented as a thin wrapper
around basic_pointerbuf. Otherwise, you've got unnecessary code bloat,
since all the code is duplicated for each value of N.

----------------------------------------

<boost/io/pointer_stream.hpp>:

I think pointer streams and stream buffers are moderately useful, and
would vote for acceptance, but I have the following objections.

1. Having separate classes for const and non-const pointers leads to
far too many templates. Const-correctness can be enforced by
the stream buffer by not allowing constructors taking const char* to
set the put area.

2. The interface is strange and poorly documented. For example, the
specification of begin_pointer and end_pointer refer to the 'utilized
array-segment'. What does this mean? Looking at the source, I see that
they return pointer delimiting the get area, if the get area is valid,
and pointers delimiting the put area otherwise. Why not simply make
them return the pointers passed to the constructor, and call them
begin and end? Together with the current seek positions -- available
using pubseekoff -- this should give the user all the necessary
information in a less confusing way.

Here's how I handle the situation in my library. There are three
light-weight templates basic_array_source, basic_array_sink and
basic_array_resource, for accessing arrays in read-only, write only
and read-write mode. There are also typedefs array_source, array_sink,
... . You can now defined array streams as follows:

     typedef stream_facade<array_source> array_istream;
     typedef stream_facade<array_sink> array_ostream;
     typedef stream_facade<array_resource> array_iostream;

These streams have a simple open/is_open/close interface like
std::fstream:

     char buf1[1000];
     char buf2[10000];

     // Write to buf1:
     stream_facade<array_sink> out(buf, buf + 10000);
     out << "hello first array!" << std::ends;
     out.close();

     // Write to buf2:
     out.open(buf2, buf2 + 10000);
     out << "hello second array!" << std::ends;
     out.close();

     // Read from an array:
     const char* str = "hello array!\n";
     streambuf_facade<array_source> in(str, str + std::strlen(str));
     std::cout << &in; // Writes "hello array!"

----------------------------------------

<boost/io/null_stream.hpp>:

I don't understand the stated rationale for this stream. Are you
saying someone might want to use it to implement a command-line
interface?

An alternative rationale would be this: sometimes a C++ interface
requires that the user pass an ostream to receive output, but the user
doesn't care about the output. I have seen a request for a stream like
this on comp.lang.c++, and I think it would be a moderately useful
component. Using my library it can be implemented as follows (as a
narrow-character sink, for simplicity):

     struct null_sink : sink {
         void write(const char*, std::streamsize) { }
     };
     typedef stream_facade< null_sink<char> > null_ostream;
     etc.

This actually allows a null stream to be buffered, which improves
efficiency even if you're going to throw the output away.

----------------------------------------

<boost/io/value_stream.hpp>:

As above, the rationale doesn't make sense. Unlike above, however, I
can't think of a reasonable alternative rationale. BTW, it can be
implemented like this using my library (as a narrow-character source,
for simplicity):

     struct value_source : source {
         char val;
         value_source(char val) : val(val) { }
         std::streamsize read(char* s, std::streamsize n)
         {
             std::memset(s, (unsigned char) val, n);
             return n;
         }
     };
     typedef stream_facade<value_source> value_stream;

     value_stream in('a');
     char c;
     in >> c;
     assert(c == 'a');

------------------------------------------

<boost/io/iomanip.hpp>

- basic_ios_form: I don't understand what this template is supposed to
do. You need a better description and some examples. Otherwise, I vote
to reject.

- resetios: I believe this would be useful.

- skipl, multi_skipl: These sound useful too, though I agree with
Pavel that the names are too cryptic.

- multi_skipper: I'm not sure I see why this is needed; how
often do you need to skip a sequence of repeated characters? Id rather
see a utility that skips a certain specified sequence of characters,
and sets failbit otherwise, as described by Dietmar Kuehl here:
http://tinyurl.com/3jzwg

- multi_newer: How often do you need to write multiple copies of a
character to a stream? And isn't it easy to do so already? The name is
bad, too. I vote to reject.

- multi_newl: Same as multi_newer. I don't see the need for inserting
long sequences of newline characters.

- newl: This is a good idea. But I think this may be one of the rare
components which I would like to see added to the standard library but
not to boost. If it were part of the standard, every C++ programmer
would be expected to know what it means. But I can't see myself
including a boost header just to avoid writing "\n": most people
won't know what newl it means, so rather than being self-documenting
using it would be self-obfuscating. I vote to reject.

Best Regards,
Jonathan