Thorsten Ottosen wrote:
Here are some general comments: ----------------------------------
0. I think the library should be put in the directory "boost/output_format" I hate cryptic names.
Or "boost/ioformat" since it also has input capabilities :). There was talk when I was developing the library about moving it into the "boost/io" directory, but that seems to be getting a little crowded ;)
1. In this example
std::cout << boost::io::formatob( vec ); // output: [ 1, 2, 3 ] std::cout << boost::io::formatob( vec ).format( "{ ", " }" ); // output: { 1 : 2 : 3 }
why does the change of braces change "," to ":" ?
It doesn't! This was a typeo. (Fixed in my documents, will commit post-review).
2. In this example
int a[] = { 5, 4, 3, 2, 1 }; std::cout << boost::io::formatob( boost::io::range( a, a + 5 )); std::cin >> boost::io::formatob( boost::io::range( a, a + 5 )); // output: [ 5, 4, 3, 2, 1 ]
it might be good to support a range version with the posibility to
1. throw if there is too few elements 2. default initialize if there is too few elements
You mean when reading in the input? The default behaviour is to set the input stream fail bit, so you can check that.
3. cryptic name: formatob.hpp && formatobex; should be format_object and whatever the latter really means
I was going for ease of typing :). maybe boost::io::format/boost::io::formatex. The latter allows you to control the delimeter type being used, so you can do: std::wout << boost::io::formatobex< const wchar_t * >( v ).format( " : " );
4. given std::cout << boost::io::formatobex< std::string >( v );
maybe std::string should be a default argument?
I was thinking here about performance. The default type is const char * which should be fairly flexible without providing compy penalties.
5. A lot of boost::io::range() can be replaced with one Taking a Single Pass Range
You mean from your Boost.Range library? This is something that I am considering, but don't know enough of the library to do this yet.
6. STL IO: It is not obvious how insertion is done? With push_back or by overwriting elements?
The container is cleared and insert is used to insert the elements into the container.
7. why is it necessary with boost::io::formatob( boost::io::range( a, a + 5 )); could we not just say boost::io::range( a, a + 5 );
Because the boost::io::range_t class (created by boost::io::range) does not provide input/output functionality. It is a holder for a range [first, second) so that boost::io::formatob can process it. This is if you either specify a std::pair (boost::io::formatob treats this as a 2-ary type and not a range holder) or a (first, last) pair of values (since boost::io::formatob accepts a single object). It could be possible to make boost::io::range return a boost::io::formatob_t type (and thus allow the above), but when I previously used seperate constructs, it interfered with the type/format deduction mechanism.
8. It should be possible to say
vector<int> v = ...; cout << v;
without the format_ob() if I include stl.hpp, ie, overloads for all standard types.
Yup. This will provide the default formatting ("[ a, b, c ]" in this case). If you need alternate formatting, you need to use boost::io::formatob.
9. I would prefer basicfmt_t to be basic_format_type or perhaps basic_fmt_type if the "fmt" shorthand is used consistently etc. That is, all the cryptic concatenation should be removed.
That would make things fairly long-winded. For example: std::pair < std::pair< int, std::vector< char > >, std::list< std::complex< float > >
mct;
std::cout << boost::io::format_object( mct, boost::io::pair_format ( boost::io::pair_format ( boost::io::basic_format(), boost::io::container_format() ), boost::io::container_format( boost::io::nary_format()) ) ); compared to: std::cout << boost::io::formatob( mct, boost::io::pairfmt ( boost::io::pairfmt( boost::io::basicfmt(), boost::io::containerfmt()), boost::io::containerfmt( boost::io::naryfmt()) ) );
And now my review comments.
| When doing the formal review, please answer the following questions: | | 1. What is your evaluation of the design?
seems ok. I'm a little irritated about too cryptic names, concatenation and _t types.
Basically, the "_t" types indicate the implementation type of a given construct. For example, the boost::io::formatob function creates an instance of the boost::io::formatob_t class. With regards to names, I am intending on changing FormatTraits to DelimeterTraits and possibly a few others. I am not sure what you mean by concatenation.
| 2. What is your evaluation of the implementation?
haven't looked.
| 3. What is your evaluation of the documentation?
good, although I miss return-types and template paramaters and some synopsises with better overview. For example, what does boost::io::formatobex< DelimeterType >( const T & ob ); return? Maybe it doesn't matter, but then it should say <i>Implementation-defined</i>boost::io::formatobex< DelimeterType >( const T & ob ); etc.
Okay, I will bear that in mind. FYI, the above function returns: formatob_t< T, DelimeterType, typename deducer< T, DelimeterType
::type::outputter > ! My rationale for not supplying the return types was to not consume more space and not confuse readers. Marking them *implementation defined* would be a good idea.
| And most important, | 8. Do you think the library should be accepted as a Boost library? | Be sure to say this explicitly so that your other comments don't obscure | your overall opinion.
yes. I think all issues can be dealt with quite easy for a post-review.
Great. Regards, Reece _________________________________________________________________ It's fast, it's easy and it's free. Get MSN Messenger today! http://www.msn.co.uk/messenger