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

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2004-08-22 13:19:30

"tom brinkman" wrote:

> The review of Daryle Walker's "More IO" library
> begins today, August 21, 2004.
>

I took look on the library. It definitely contains
useful functionality, but:

 a) its documentation is bad (details bellow) and must
    be rewritten (or rather actually written),

 b) none of tests compiled, due to bugs in code,

 c) it may be considered to use inline definitions everywhere
    instead of out-of-class definitions. It would make code
    shorter, easier to grok and friendlier to older compilers.

 d) more functionality may be considered, see item [25] bellow

 e) naming conventions may be reconsidered (see item [5])

I recommend to fix points (a) and (b) first.
Then and only then it should be added to Boost,
not in current state. (It would not require another
full review, IMHO.)

More issues and nitpicks are listed bellow.

/Pavel

_______________________________________________________________
1. docs: ios_state.html is missing.
   Dtto ios_state.hpp.

_______________________________________________________________
2. docs: some introduction into the library would help.

_______________________________________________________________
3. docs: source code snippets should be syntax colorized

_______________________________________________________________
4.

#if (defined _MSC_VER) && (_MSC_VER >= 1200)
#  pragma once
#endif

should be put into each of headers. It has
positive effect on VC ad Intel C++ compilation times.

_______________________________________________________________
5. naming conventions: some names as

       basic_constpointerbuf

   are quite hard to read. Maybe they should

       basic_const_pointer_buf

   (This is just subjective opinion of me.)

_______________________________________________________________
7. stream_buf_wrapping.hpp: there are three hardcoded
   constructors like:

    template < typename T1, typename T2, typename T3 >
    basic_wrapping_ios( T1 x1, T2 x2, T3 x3 )

   for 1, 2 and 3 parameters though base_from_member
   supports up to 10 parameters.

   Boost.Preprocessor can be used here to generate
   contructors of all available arities.

   Likewise the basic_wrapping_XYZ bellow.

_______________________________________________________________
8. stream_buf_wrapping.hpp and others, a nitpick:

   The name BOOST_PRIVATE_WRAPPER may be
   more unique, e.g. BOOST_IO_PRIVATE_WRAPPER
   to avoid  any chance of collision.

_______________________________________________________________
9. stream_buf_wrapping.hpp: what is rteason for line:

     using pbase_type::rdbuf; \

   Doesn't seem needed.

_______________________________________________________________
10. array_stream.hpp: the template

    template < std::size_t N, typename Ch, class Tr >
    class basic_array_##SuffixF

   may contain static assert N > 0.

_______________________________________________________________
11. array_stream.hpp: the constructor

template < std::size_t N, typename Ch, class Tr >
inline
basic_array_streambuf<N, Ch, Tr>::basic_array_streambuf
(
    typename basic_array_streambuf<N, Ch, Tr>::char_type const *  b,
    typename basic_array_streambuf<N, Ch, Tr>::char_type const *  e,
    std::ios_base::openmode  which
      // = std::ios_base::in | std::ios_base::out
)
...

  may contain
  BOOST_ASSERT(b != 0);
  BOOST_ASSERT(e != 0);

_______________________________________________________________
12. array_stream.hpp: what is reason to use this, e.g. in:

    return this->gptr() ? ( this->gptr() - this->eback() ) : 0;

?

_______________________________________________________________
13. array_stream.hpp: function seekoff(): shouldn't

if ( newindex > off_type(self_type::array_size) )  goto bail;

be

if ( newindex >= off_type(self_type::array_size) )  goto bail;

?

_______________________________________________________________
14. array_stream.hpp: the InputIterator in

    template < typename InputIterator >
    basic_array_streambuf

   can be 'concept-checked'.

_______________________________________________________________
15. iomanip_form.hpp: in

    explicit  basic_ios_form( ::std::basic_ios<Ch, Tr> const &i );

    the parameters really should not be named 'i'.

_______________________________________________________________
16. iomanip_general.hpp: is the declaration:

    template < typename Ch, class Tr >
    std::basic_ios<Ch, Tr> &  resetios( std::basic_ios<Ch, Tr> &s );

    needed?

    Likewise in iomanip_io.hpp the declaration of
      - multi_skipl()
      - skipl()
      - operator >>()
      - etc

    I would prefere smaller file w/o them.

_______________________________________________________________
17. iomanip_general.hpp: operator>>: I find the expression:

       return s( is ), is;

    abominable.

_______________________________________________________________
18. streambuf_wrapping.html: some sentences are rather hard
    to parse:

    "...checking if the stream buffer's stream switched to
     an external stream buffer."

    Maybe its possible to rearrange them so 'stream' and 'buffer'
    will be less frequent in the text.

    The term "switching" used here should get explained.

_______________________________________________________________
19. array_stream.html: the documentation should contain some
    example code snippet. Now it is just source code annotated.

_______________________________________________________________
20 iomanip.html: it should be explained what is "form-based".

   The documentation should NOT be annotated code, it is
   very unreadable.

   Code snippets examples are missing.

_______________________________________________________________
21. naming: the skipl should be named skip_line or so.
    Current name is extremely annoying for any code maintainer.

    The fact that endl is in standard doesn't mean cryptic names
    should be piled up.

_______________________________________________________________
22. value_stream.html: the rationale here is copy of rationale
    from null_stream.html and completely inapropriate, IMO.

_______________________________________________________________
23. compiling tests on Borland C++ Builder 6.4:

--------------------
array_stream_test.cpp: doesn't compile because
  typedef boost::io::basic_array_streambuf<alphabet_length, char>
astreambuf;

contains only 2 template parameters instead of required 3.

--------------------
iomanip_test_io.hpp: doesn't compile because BCB has problems
with the template declarations in iomanip_in/out.hpp.
When I commented them out BCB stopped to complain.

Failure in:
class sync_count_stringbuf {
   typedef base_type::allocator_type  allocator_type; <<== this doesn't
compile

--------------------
null_stream_test.cpp:

null_stream_test.cpp(28): error: namespace "boost::io" has no member
"nullbuf"
      boost::io::nullbuf  nb;

--------------------
pointer_stream_test.cpp:

pointer_stream_test.cpp(31): error: namespace "boost::io" has no member
"pointerbuf"
      boost::io::pointerbuf  pb( buffer, buffer + buffer_length );

--------------------
value_stream_test.cpp:

value_stream_test.cpp(29): error: namespace "boost::io" has no member
"valuebuf"
      using boost::io::valuebuf;

_______________________________________________________________
24. compiling tests on Intel C++ 7.0 plugged in VC6 IDE:
    none of the tests compiled, for the same reasons as with BCB.

_______________________________________________________________
25. possible features: there may be more features in library:

 - 'tee' like output stream, they take data and re-send
    them all into two other streams

 - merging input stream which reads data from one stream, when
   it is exhausted then other stream etc.

 - in/out stream buffers which adapt some container, e.g.
      container_ostream<std::vector<char> >

 - out stream buffer which uses fixed size buffer and if more data
   are inserted into, then it allocates additional buffer from heap.

 - 'annotating' out stream, for example adding current time to
    every data item put in. It may have a functor as parameter
    and this functor would produce the annotations.

 - filtering in/out stream which uses binary predicate functor
   to remove data from stream.

_______________________________________________________________
EOF