On 8/22/04 2:19 PM, "Pavel Vozenilek" <pavel_vozenilek@hotmail.com> 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