[boost] Re: Formal review of "Output Formatters" library begins today

From: Vladimir Prus (ghost_at_[hidden])
Date: 2004-09-20 10:57:48

John Torjo wrote:

> The FORMAL Review of "Output Formatter" library begins today,
> Sept 12, 2004.

Before going to review itself, I'll list the cases where I want to use this
library. 

The primary case is debugging. I want to either output small vectors to some
log/dump file (in which case the output will be small one-line), or output
some huge structures (vector<Function>, where Function has a lot of data).
In the latter case, the output should be multiline, with nice indentation,
or I won't understand anything.

The second case is STL I/O itself, for example csv files that Reece has
mentioned.      

A particularly interesting question is how the proposed library overlaps
with serialization. When outputting vector<Function> I'd prefer the content
of 'Function' to be outputted too, preferably by describing the members
with the 'serialize' method. And the question is if I can use outfmt
library, or have to use the serialization library? The serialization
library is pretty large, so, I'd like outfmt to be able to output UDT which
have 'serialize' defined. IMO, vector<some_UDT> is a very common case,
maybe even more common than vector<pair<int, int> >.

> 1. What is your evaluation of the design?

Unfortunately, I cannot comment much on this due to bad documentation. See
below.

There are some things I don't like. First, as many mentioned, the naming is
not optimal. For example:

   boost::io::formatob(v).format(" | ");

The 'ob' suffix and two "format" words are confusing. I'd suggest something
like:

  boost::io::stl(v).separator(" | ");

or 
    
  boost::io::separate_with(v, " | ");

and for braces something like

  using namespace boost::io;
  braces("[", "]", separator(" | ", v))

or maybe even

  ("[" + io(v)/"," + "]")

for nested formats, something like

  ("[" 
      + io::format(
         "(" + io::format()/":" + ")"
        )/","
      +
    "]"
   )

These are just ideas, though. 

It's desirable that the library support some multiline output style out of
the box, so that I could write:
 
  os << io::multiline(v) << ...

Again, I'd suggest YAML as such style, but any other readable indented style
will be OK. It's also desirable to use the 'serialize' method of classes to
output them, instead of requiring operator<< to be always present.

> 2. What is your evaluation of the implementation?

There should be the <boost/outfmt.hpp> header, including everything else of
the library.

Some of the lines are longer than 80 characters (e.g. template header of
formatob_t has 111 characters).

Methods defined in the body of the class are implicitly inline, there's no
need to put the "inline" in front of them, as it's just one more word to
read.

I don't think defining methods in the class is a good idea -- this makes the
class interface less obvious. Another issue is that all of methods are
inline, and I don't think it's necessary. The iostream operations will take
much more time than function call anyway, and inlining everything can lead
to code bloat. For example list_object::read is definitely very large
method.

Why is the formatob_t necessary? It seems to work by delegating everything
to the underlying formatter. Can't the 'formatob' just return the
appropriate formatter?

I might be missing something, but the mechanism for getting the type of
formatter from a type to be output seems too complex. First, the
type_deducer.hpp file is used, and 'select' computes a 'category'. Then
format_deducer.hpp takes the category, and again uses 'select' to obtain
the real type of formatter. Why the type_deducer.hpp is needed? 

> 3. What is your evaluation of the documentation?

The documentation leaves much to be desired. I'll walk though some of the
aspects (indented text is quote from the docs)

First, about the structure. I'd prefer more conventional
introduction/detailed docs/reference structure. The current docs, IMO, mix
everything, and when reading sequentally, I find both vague phrases and
exact synopsis, and very little overview or tutorial material.

     "providing an extensible framework that sits"

How it's extensible? I see only one section about extending and it's just
one paragraph long

    It is often necessary to override the way a type is formatted (written   
    to/read from) to a stream, or to format a subrange of a container or
    array. The manipulators in this library serve this purpose. For 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 ));

I don't think I see a conventional meaning of "manipulator" here. The
formatting style is not changed at all. Just a 'range' function is used to
create a container from an interator range.

 boost::io::formatobex< DelimeterType >( const T & ob );

 This will format ob according to it's underlying type.

How is this different from using the 'formatob' function. What's "underlying
type". The "This will format" phrase is very loose, it does not even say
anything about stream. I'd suggest: the returned object can be output to a
stream, and will produce textual representation of 'ob', or something
equally explicit.

 boost::io::formatob( const T & ob );

 This will format ob according to it's underlying type.

I'd suggest that this is documented before 'formatobex'. I'd also suggest
for formatobex use the "basic" prefix to indicate it's templated on the
character type. E.g. stlio and basic_stlio, or something like that.

   std::cout << boost::io::formatobex< std::string >( v );
   // output: [ 1.1, 2.2, 3.3 ]

  Note that the type construct is automatically deduced and the
  corresponding format object is constructed. 

Again, this is very vague. What does it mean for "type construct" to be
automatically "deduced". Why would I care about some "format object". You
might want to say that under the cover, the type of 'v' determines the type
of format object that's created, and that the object is responsible for the
actual formatting.

     This will format ob based on the format object it is passed
     (FormatObject). Here, the format type is taken from
     FormatObject::format_type. This allows the nested constructs to be
      formatted.

This is not clear at all. What's 'format type' and how it's "taken". How it
allows the nested constructs to be formatted. I think you need to either
elaborate on this, or move this passage to a detailed docs section.

   If you need to specify a range or sub-range, boost::io::formatob will not   
   recongnise it unless it is a container. 

   boost::io::range( ForwardIterator first, ForwardIterator last );

This is more of a language problem. Range, by definition, is a pair (but not
std::pair) of iterators. It's never a container.

    This creates the range [first, last) that can be used by
     boost::io::format. 

The range [first, last) exists as long as you have the 'first' and 'last'
objects. I think it's better to say: "Returns an object which will output
the range [first, last) to the stream....". The remainder of the "range"
function overload leaves me wondering if this is reference or overview or
what. For a reference, the code examples are not necessary. For tutorial,
you don't need to list every overload, just mention that there are others.

   A FormatTraits class is a class that provides the default values for the
   open, close and separator delimeters used when rendering the list to a
   stream. These can be overriden by the user as described in the following
   section. The class has the general form: 

This sentence doesn't say why FormatTraits is usefull for a user. I read it
like: the class provides default values, and user can override those value
when outputting a specific object. It does not seem that the user can
specialize the class for his objects, so why the user should know about
this class at all? Heh, both provided traits are in the detail namespace?
Really, user should not care.

   boost::io::openclose_formatter is a class that allows the user to change
   and access the format used for open and close delimeters

Here we're definitely in the reference docs already, while I did not get an
overall picture yet. Then, what's "change and access the format". If the
format can be changed, it is stored somewhere. Where? The code block before
this comment defines two classes openclose_formatter_t and
openclose_formatter. Is this a typo, or you really have two classes?

   boost::io::openclose_formatter_t is used by format objects so that a
   reference to the format object is returned and not a reference to
   boost::io::openclose_formatter! This makes it possible to call the format
   function inline. For example: 

It's completely unclear. Then you go on describing the openclose_formatter
class, while I still don't know how I can use that class.

   Formatters are useful when you want to use a specific format in different   
   places, for example: 

This should be the first sentence in this section.

   The FormatObject class has to provide a write function having this
   syntax: 

   FormatObject::write( os, fo.ob );

Maybe, you mean "The FormatObject class has to be written such that
expression FormatObject::write(os, fo.ob) is well formed". My first
impression was that you give a signature for the write method.

   The write function has the form: 

   template< typename T, class OutputStream >
   inline OutputStream & write( OutputStream & os, const T & value ) const
   (
      // ...
      return( os );
   );

Do you mean that every FormatObject class should have this signature? Then,
the phrase about "FormatObject::write(os, fo.ob)" is not necessary.

> 4. What is your evaluation of the potential usefulness of the library?

Potentially very usefull.

> 5. Did you try to use the library?
>     With what compiler?  Did you have any problems?

No.

> 6. How much effort did you put into your evaluation?
>     A glance? A quick reading? In-depth study?

A couple of hours.

> 7. Are you knowledgeable about the problem domain?

Kind of. I wrote a similar library a few years ago, though much less simple.

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

I'd a hard question. The basic idea of the library -- formatter objects that
you can nest, seems sound to me. However, the interface built on top of
that is a bit suboptimal, and the state of documentation is pretty bad.

Because of that, I believe that it would be better if the library is
rejected this time, and reviewed again later.

- Volodya