"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