Re: [boost] Ongoing review of printf-like formatting

From: Jeremy Siek (jsiek_at_[hidden])
Date: 2002-01-20 22:47:49

This review is based on the format.zip file dated 1/20/2002.

I'm in favor of accepting Boost.Format. It's a nice solution to a common
task that is clumsy with plain ostreams and not very safe with printf (or
compatible with streams). I'd like to see some more polishing on the docs,
examples, and tests, but on the whole it looks good.

Oh, and I'm slighly in favor of % over [].

Here's all my nits:

Comments on the documentation:

index.htm
  Looks like the description was a cut and paste from the timer class. Oops.

  The link to sample_formats.cc is broken (both here and in format.html)

  Should link to the other example files and the test files. It would be
    great to have short summary of each file.

format.html

  "The <boost/format.hpp> class"
  That's a header file, not a class.

  You have two "Synopsis" sections. How about just deleting the
  first "Synopsis" heading.

  Need more examples at the very beginning. The one example is nice,
    but we need about 5 one or two line examples. Also it would be nice
    for motivational purposes to show how several example look using
    plain old ostreams, printf, and Boost.Format to show off the
    benefits of Boost.Format.

  I would slightly prefer the name "to_string" instead of "str".

  printf directives: need some examples. Pretend the user has
    never seen printf (I know this may seem silly, but consider
    a student who learned Java first, and has never heard of printf).

  "glue" is a bad name for this function. How about something like
     "modifier" or even "format" instead (or some other better name).

  Under "Differences of behaviour vs printf"
    "pformat" is mentioned. What is "pformat"? It has not been introduced.
    Did you mean just "format"? Nope... looking at the examples it is
    something different... means use non-positional directives.

Some general questions:

  What if the wrong number of arguments are passed?
       Answer: throw an exception. This needs to be documented.
  What happens if you use format specifications for numbers, and then
    the argument is not a number?
  How's the performance? Ahh, there's a benchmark file. How about
    creating an HTML page to tell people what you've found.

Portability issue: Boost.Format uses sstream (which bad for older g++'s)

Comments on the Code:

  tried to compile with g++ 2.95.2, which doesn't have sstream, so
    that didn't work.

  Compiled sample_formats.cc with g++ 3.0.2, -Wall and -pedantic.
    It compiled cleanly.

  trying to compile sample_formats.cc with KCC, got the following
    warnings and errors. I've only listed each kind of error once.
    They also occur on other lines. I would pay attention to these
    errors. The EDG frontend (used by KCC) it highly standards
    conformant, so these errors are likely problems in the code,
    not in the compiler.

  "sample_formats.cc", line 28: warning: temporary used for initial value of
            reference to non-const (anachronism)
        oss << format("writing %3,  x=%1 : %2-th step \n") [40.23] [50] ["toto"];

  "../../../boost/format.hpp", line 208: error: type
          "boost::format::basic_format<charT, Traits>::stringstream_t [with
          charT=char, Traits=std::char_traits<char>]" is inaccessible
      typedef typename basic_format<charT, Tr>::stringstream_t   stringstream_t;

  "../../../boost/format.hpp", line 209: error: type
          "boost::format::basic_format<charT, Traits>::string_t [with
          charT=char, Traits=std::char_traits<char>]" is inaccessible
      typedef typename basic_format<charT, Tr>::string_t   string_t;
                                                ^
  "../../../boost/format/format.ihh", line 448: error: more than one instance of
          overloaded function "std::basic_string<charT, traits,
          Allocator>::insert [with charT=char, traits=std::char_traits<char>,
          Allocator=std::allocator<char>]" matches the argument list:
            function "std::basic_string<charT, traits,
                      Allocator>::insert(std::basic_string<charT, traits,
                      Allocator>::size_type, std::basic_string<charT, traits,
                      Allocator>::size_type, charT) [with charT=char,
                      traits=std::char_traits<char>,
                      Allocator=std::allocator<char>]"
            function "std::basic_string<charT, traits,
                      Allocator>::insert(std::basic_string<charT, traits,
                      Allocator>::iterator, std::basic_string<charT, traits,
                      Allocator>::size_type, charT) [with charT=char,
                      traits=std::char_traits<char>,
                      Allocator=std::allocator<char>]"
            argument types are: (int, int, char)
          res.insert(0, 1, ' '); // insert 1 space at  pos 0
              ^

  In the examples, no need for the asserts. Examples are not tests.

  In sample_formats.cc:
    Could use more comments saying what the printf directives mean.

    Ahh, now we find out what happens with too few or too many arguments.
    An exception is throw. This needs to be documented, including the
    type of exception so the user knows what to catch!
    Hmmm, maybe this should be an assert instead of an exception?

  In sample_advanced.cc:

    First example, talk about "glue" but then glue is not used, just
     operator(). Did the comments and docs not get updated after a
     change in interface?

    I'm seeing a bunch of functions getting used that I didn't see
      in the documentation:

      set_flag
      bind_arg
      clear
      clear_binds

  In sample_userType.cc:
      Is the operator<< for Rational written correctly?
      Perhaps write a HTML page that walks the user through this example.

      I noticed the real boost rational class does not handle the
      formatting flags properly. Let's make sure that gets updated.

  In format.hpp:
     The docs said the format class was at boost::format. In the code I see
     boost::format::format.

     clear_bind. I don't think this one was convered in an example,
       and as with a bunch of the other functions, it was not documented.

     What are the exceptions() functions all about? They need to be
       documented?

Testing:
     Did every member function of the format class get tested?
     Did they get tested in all the important situations?
     From the test files, it looks like the basic formating stuff
       is tested, but not the other member functions.
     The docs should summarize the test coverage and relate
        the tests to the user interface.

----------------------------------------------------------------------
 Jeremy Siek                          http://www.osl.iu.edu/~jsiek/
 Ph.D. Student, Indiana Univ. B'ton   email: jsiek_at_[hidden]
 C++ Booster (http://www.boost.org)   office phone: [phone]
----------------------------------------------------------------------