"format" review comments

From: Darin Adler (darin_at_[hidden])
Date: 2002-01-21 02:55:08

I reviewed version 1.30 because it was marked "review candidate".

I vote that we accept this library into Boost.

While there are significant problems with the version I reviewed, I think
the basic approach is great, and this is a sorely needed set of features.

On the other hand, the documentation is currently in very rough shape.
Besides editorial problems, there are major features, like the bind_arg
feature, that aren't documented at all. The code is unnecessarily cryptic
and formatted inconsistently, thus difficult to review.

Major comments

I think that pformat and wpformat should be the default, and the version
that supports the "%1" shortened format should be either removed, or made
available in the less-obvious name. People using this library for the first
time are the most likely to want the printf-compatible format, so it should
be the thing you are most likely to run into, not a 2nd-class feature. Maybe
the ones that take the shortened format could be named fmt and wfmt.

The documentation gives high-level information, but doesn't document all of
the public entities defined in the library. For example, it doesn't document
the basic_format class template much at all, doesn't define what the
short_notation parameter is, doesn't even mention the bind_arg feature. Most
of the public members are omitted from the basic_format synopsis.

I'm a bit uncomfortable devoting the symbol "boost::glue" to the boost
formatting library. It seems like a name that's not particularly
formatting-specific.

Minor comments

It's excessive to have *both* a free function and a class member for the
str() feature.

The documentation does not explain how to make a format string that outputs
the value of the first argument, followed by the digit 1. "%11" won't work.
The classic printf syntax does not have this problem.

When trying to compile format.hpp, I ran into about 12 places where it uses
size_t and it should use std::size_t. These should be fixed.

When trying to compile format.hpp, I ran into about 6 places where it uses
size_t and it should use std::isdigit. These should be fixed somehow.

In put(), the call to res.insert(0, 1, ' ') is ambiguous, so won't compile.
It needs to be res.insert(static_cast<string_t::size_type>(0), 1, ' '). This
also comes up in do_fill(), and a few other places where insert is called
with the first parameter 0. A search for "insert(0" finds them all.

It would be nice to have a plain full word equivalent to str(). I like
make_string() myself for the free function. But I'm partial to normal words
and verbs for function names. For the member function, maybe just string().
If pressed, I'll admit that such functions are called AsString() in my
programs, but that doesn't mean I like this convention, which I adopted many
years ago.

I am uncomfortable with the use of the ".ihh" extension for a sub-header
file with the implementation details. If a separate implementation header is
needed, I would suggest a name like <boost/format/format_detail.hpp>,
<boost/format/format_implementation.hpp>, or <boost/format/format_impl.hpp>.

Since there's a separate implementation file, I suggest moving the
format_internals contents in there, except for the forwards .

The documentation specifically says <boost/format/format.ihh>, but the code
says "boost/format/format.ihh".

The documentation needs copy editing. I mention as many things as I can find
in a quick pass here, but I think a more careful pass is needed by someone
empowered to fix things.

In the documentation, there is much misspelling of "formatter" as
"formater". A global replace is in order.

Boldface on the word "forbidden" is uneeded, and not good style.

The style xxx_t is used for typedefs. But xxx_type is nicer.

The string_t typedef is private, but that type is used in the basic_format
constructor.

The word "bound" should be used rather than "binded".

In general, there's haphazard use of italics and bold in the documentation,
and word emphasized in different ways, like "***not***".

It says "A Printf specification" but it should say "A printf specification".

In the documentation, the word "tedious" is misspelled "teddious".

The documentation should not use ".." to end sentences to indicate
uncertainty. A simple "." will do.

It should say "e.g.", not "eg".

It should say "C++", not "c++".

The phrase "might not be intuitively right" needs to be rewritten; I don't
think it means what you think it means.

The documentation does not do a good job documenting exactly what is
different from The Open Group printf format strings. The section
"Differences of behaviour vs printf" documents some of these differences,
but the extensions to printf syntax are elsewhere.

In the documentation, it says "But so does the 0 and '' options" and it
should be "But so do the [...]".

The samples should use ".cpp" extensions, not ".cc" extensions.

The "sample_userType.cc" should be named "sample_user_type.cpp" instead of
mixing naming conventions.

The link to "sample_formats.cc" is broken.

The description at the top level describes the timer library.

There should not be a commented-out include of <boost/config.hpp>.

The "operator string_t ()" should be removed completely, not just commented
out.

The pformat() and wpformat() functions should be overloaded for const
string& and const wstring&.

The public member functions of basic_format aren't documented.

Should set_flag be named apply_manipulator or set_manipulator (or
apply_manip or set_manip)?

Is there a need to use the names glue_ref and glue_cref in the global boost
namespace? Aren't these secret implementation details?

The use of "signed int" in the code seems archaic and strange to me. You
should just say "int", even when casting from unsigned to signed.

In the code, there's a typographical error "convertion" instead of
"conversion".

The str2int function has undefined behavior when there's a sequence of
digits too long to fit into an int. That should be fixed. A symptom of this
is that on my system, 'cout << format("%4294967297") % 1;' does not throw an
exception.

Using standard English, it would be "clear_all_bound_args" and
"clear_bound_arg" rather than "clear_binds" and "clear_bind".

Using "bound_.size() == 0" as a flag to indicate whether any arguments are
bound makes for confusing code. A separate boolean would be clearer and I
don't see any downside.

There seems to be a lot of repetitive code. In particular the two operator%
functions shouldn't be cut and paste jobs. They should share code.

The inline keyword is used inappropriately. Marking large functions like
operator% and compute_states and str2int inline does not make sense.

Why the "using namespace std" inside parse()?

There really should be a test program or section of a test program that
tests against printf to see that the same format strings give the same
results from pformat and sprintf.

The HTML overuses "<br>", making it very hard to read. Paragraphs should be
enclosed in "<p></p>" and "<br>" should only be used in unusual
circumstances.

I'd prefer that the header include <istream> and <ostream> rather than
<iostream>.

The source files use tabs.

The documentation says that asterisks in format specifiers are "quietly
ignored". It's unclear if that means that the corresponding parameter is
skipped or not.

Dangling "private:" in format_item.

    -- Darin