RE: [boost] Re: Formal review of "Output Formatters" library beginstoday

From: Reece Dunn (msclrhd_at_[hidden])
Date: 2004-09-12 09:49:38

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