Re: [boost] "format" review comments

From: Samuel Krempp (krempp_at_[hidden])
Date: 2002-01-21 06:18:50

On Mon, 2002-01-21 at 08:55, Darin Adler wrote:

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

what about boost::format::manip instead of boost::glue ?

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

that issues is fixed in my local versions, by using brackets :
"%{1}1"

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

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.

fixed

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

true. I was surprised that there be an implicit conversion from 0 to an
iterator type used in another form of insert.

The full fix looks uglier than necessary :
static_cast<typename string_t::size_type>(0)
because we need a typename, since string_t actually depends on template
parameters. (only 80%  sure about this..)

I'm wondering if 
std::size_t(0)  is enough to insure the overload resolution is no more
ambiguous..
It would be prettier.

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

the headers structure seems unpopular.
Rogeef (http://groups.yahoo.com/group/boost/message/23405) think I
should cut the code into smaller headers.

Apparently, MSVC compatibilty requires, among other things, that I
define template members inside the class definition,
thus I'll need to redesign the whole structure.

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

Well, I noticed boost headers were included with '< >' in all boost
libraries.
Personnally, I don't like that since my makefiles assume it means the
headers won't change, and thus is not put into the dependancies..
(I guess other people have the same kind of side-effect with boost's <>
includes)

I thought I would do like other boost libraries, but somehow I ended up
replacing the <..> with ".." in my code, and forgot to change it back
before uploading.

Is it a required boost policy, to use #include <boost/format.hpp>, 
or can I choose #include "boost/format.hpp" ?

Your minor comments are very helpful, and I'm now working to correct the
errors you spotted.

Thanks,
-- 
Samuel