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

From: Daryle Walker (darylew_at_[hidden])
Date: 2004-08-27 21:02:20

On 8/22/04 2:19 PM, "Pavel Vozenilek" <pavel_vozenilek_at_[hidden]> wrote:

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

As someone else stated, those are already in Boost.  (I just copied the
existing "io_fwd.hpp" when creating this one.)

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

This is a collection of many little libraries; I not sure that a grand
introduction could be made.

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

Even if that's a good idea, these are manually-created HTML files, so you
have to wait until some automation is added.

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

I'm not using Visual C++, Windows, or even a IBM-style PC, so I don't want
to go crazy with cross-platform conveniences that I can't check.  I do want
to know about cross-platform bugs, hopefully with fixes.

> _______________________________________________________________
> 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.)

That standard stream (buffer) classes run their name-parts together.

(What happened to entry #6?)

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

I wrote base_from_member, and I extended BFM after initially writing
More-I/O.

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

Any suggestions on how?  And should I leave the arity number available in
BFM (as a #define) so others can mirror it?

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

The two base classes define different "rdbuf" member functions.  That means
the subject class needs to explicitly mention which version it'll use.

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

A zero-size array will choke the compiler anyway.

> _______________________________________________________________
> 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;
> 
> ?

How else would I get the length of the currently-used get-buffer?

> _______________________________________________________________
> 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;
> 
> 
> ?

No.  The "off_type(self_type::array_size)" represents the one-past-the-end
value.  I know it's useless to dereference, just like in iterators, but it
needs to be a valid location for other uses.

> _______________________________________________________________
> 14. array_stream.hpp: the InputIterator in
> 
>   template < typename InputIterator >
>   basic_array_streambuf
> 
>  can be 'concept-checked'.

How?  And it is worth the extra code?  Like other templates, the current
code will choke if the given type does not meet the expected interface
(although with a nearly-incomprehensible error message).

> _______________________________________________________________
> 15. iomanip_form.hpp: in
> 
>   explicit  basic_ios_form( ::std::basic_ios<Ch, Tr> const &i );
> 
>   the parameters really should not be named 'i'.

Why not?

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

And I like forward-declaring everything.

> _______________________________________________________________
> 17. iomanip_general.hpp: operator>>: I find the expression:
> 
>      return s( is ), is;
> 
>   abominable.

I don't think you have the right filename.  Anyway, the code is hard to
misinterpret and it saves a line.

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

What do you mean by that?  I've tried to imitate the Standard's way of
describing items.  Anyway, it's hard to be precise about the actions/effects
without getting close to code when the functions are simple.

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

[This response is also for #23.]

All my stuff compiles for me.  If you can figure out what compiler quirks
(or mistakes on my part) are causing the problem, let me know.

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

All the example stream in the library so far have one thing in common: they
are based off the CORE language.  These requests could be based off the
standard library.  Those can come later, or may be supplied by the upcoming
for review "The Boost Iostreams Library" by Jonathan D. Turkanis.

Most of the items here aren't big in the grand scheme of all C++.  The
pointer-based streams are very important, however, as they allow existing
memory blocks to be used, finishing the legacy of std::strstream that
std::stringstream left unfinished.

-- 
Daryle Walker
Mac, Internet, and Video Game Junkie
darylew AT hotmail DOT com