[boost] Re: Formal Review for Boost I/O Library

From: Gennaro Prota (gennaro_prota_at_[hidden])
Date: 2003-03-06 12:15:51

On Wed, 26 Feb 2003 23:00:11 -0600, "Ed Brey" <brey_at_[hidden]>
wrote:

>The formal review for the updated Boost I/O Library begins today and runs through March 7.

Just some quick comments:

>The I/O library contains various components for use with the standard I/O library.  The components are as follows:
>  State-saving classes for various IOStream attributes.

These are already into boost, aren't they? Are you asking for a
further review? Or weren't they reviewed when they should have been?

>  Class templates to ease making streams off a new stream-buffer class.
>  Stream and stream-buffer class (templates) that use an internal array.

 
>  Additional I/O manipulator function (templates).

This part looks quite extemporaneous and incomplete. For instance, why
aren't there

  template <typename CharT, typename Traits>
  std::basic_ostream<CharT, Traits>&
  htab (std::basic_ostream<CharT, Traits>& os) {
      return os.put(os.widen('\t'));
  }

  template <typename CharT, typename Traits>
  std::basic_ostream<CharT, Traits>&
  bel (std::basic_ostream<CharT, Traits>& os) {
      return os.put(os.widen('\a'));
  }

  ... etc...

too?

As to the two manipulators provided:

- Though 'newl' and 'skipl' imitate the pattern of 'endl' I think it
would be better choosing more descriptive names, like new_line and
skip_until_newline. But, of course, these are religion issues :-)

- The implementation of skipl is erroneous as per lib issue 172; it
should be

template < typename Ch, class Tr >
std::basic_istream<Ch, Tr> &
skip_until_new_line(std::basic_istream<Ch, Tr> &  is)
{
    return is.ignore( std::numeric_limits< std::streamsize > :: max(),
     Tr::to_int_type(is.widen( '\n' )) );
}

- The test program for iomanip.hpp could be improved, I think. Better
showing that skipl can skip any character before the newline, for
instance with something like:

    // untested test program
    //

    char hello[] = "hello";

    std::stringstream  ss;
    ss << hello << "AxxxxxxA" << newl << "There";
    

    std::string  s;
    ss.seekg( 0 );
    ss >> std::setw(sizeof hello) >> hello;

    char c;
    ss >> c;
    BOOST_TEST(c == 'A');
    
    ss >> skip_until_new_line >> s;
    BOOST_TEST( s == "There" );

- in iomanip.hpp the function template declarations immediately before
the definitions just confuse the code IMO.

- as a general rule, I don't like having source file names too similar
to standard header-names (example: iomanip.hpp). BTW it tends to be
annoying to remember slight differences like <iosfdw> vs. "io_fwd.hpp"
and/or remembering whether the latter is iofwd or io_fwd.

>The stream buffer classes are new and deserve primary attention in this review.

Yes. Let me start from the documentation, which is IMHO absolutely
insufficient, especially in the rationale section. Informally speaking
(up to Daryle to improve the wording from a technical perspective) I
think the real rationale goes more-or-less along these lines:

When deriving from the standard basic_istream, basic_ostream,
basic_iostream classes, *you* should provide a pointer to a suitable
stream buffer (this is not the case, for instance, when deriving from
ifstream and ofstream because those classes already contain their
stream buffer). There are several ways to do this:

a) construct a stream buffer independently and associate it with a
stream object later

b) gather the buffer from another stream; this leads to a buffer
"shared" among several stream objects. Example:

template <class Ch, class Tr = std::char_traits<Ch> >
class my_stream : public std::basic_ostream <Ch, Tr>
{
public:

    my_stream(std::basic_ostream<Ch, Tr> & os):
      std::basic_ostream<Ch, Tr>(os.rdbuf())
    {
    
         ....    
    }

};

my_stream<char> m (std::cout);
m << "hi!\n";

Now m and std::cout share the same buffer. Sharing a stream buffer can
be useful in various situations where you want, for instance,
different format settings, or different locales, for the same stream:
in such scenarios instead of continuously switching the format
settings or the locale you use different streams which write to (or
read from) the same stream buffer

c) put a stream buffer into the derived stream; the stream buffer can
be a base sub-object or a data member. This is the typical approach
when a new buffer type is used, and _is what the following classes
help implementing_.

I've omitted, for laziness, an (obvious) example for case a).

Ok, now to the code. Sad to say, but this is the typical case where it
takes more time to understand what the existing "solution" is meant to
do than to write your own code from scratch. BTW, the fact that the
templates are identical except for the presence of 'istream',
'ostream' or 'iostream' in some places is highly suspicious. That
consideration apart, let us compare your code with what I would write
from scratch if I had to define my own stream:

struct buffer_wrapper {
    std::stringbuf buffer;
};

template <class Ch, class Tr = std::char_traits<Ch> >
class my_stream :
     virtual buffer_wrapper,
     public std::ostream {

public:
         my_stream() : std::ostream(&buffer) {}

     
};

Now convince me that the approach taken by this library is better than
the (immediate generalization) of the three lines above.

To conclude this quick review, I think the additions should not be
accepted into boost. Neither the code nor the documentation have the
necessary quality, clarity and cleanness, however there's definitely
room for improvements on both fields, which makes me encourage Daryle
to do some more work on them.

Genny.