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

From: Samuel Krempp (krempp_at_[hidden])
Date: 2002-01-21 13:38:13

> template<typename T> operator[](constT&) definition is moved in.
> template<typename T> operator[](T&) declaration is excluded 
> completely. Why would you need both? MSVC produces an ambiguity 
> errors if present ( the same everywhere further).

in case you want to format an object, of which you can not take a const
reference..
Well okay, for arguments sticking to (const T&) should not be a problem,
users should provide const ways to print their objects anyway.

but for passing manipulators, it's different :

> template<typename T> operator()(constT& manipulator)
> template<typename T> operator()(T& manipulator)
> Why would you need both? Though MSVC can't handle even one (seems 
> like MSVC bug) The only way I found that is working like his:
> template<typename T> operator()(T manipulator). Plus definition.

yeah, manipulators are quite a problem. You can't pass a function of
type 'manip' :
typedef std::ios_base&(manip)(std::ios_base&);
by const-reference.

Passing by copy is okay, and so is passing by (non-const) reference.

According to the standard, MSVC should accept the 2 overload (const and
non-const ref).
If I had declared one function with copy, and the other with const-ref,
I think the standard says there could be ambiguities. ah well this kind
of resolution of overload is really awful.

If MSVC really can't accept the current overload, I'll write a special
case for it. 
We can supply just const-reference for argument passing,
and copying for manipulators. (it looses a bit of genericity on the
kinds of manipulators it will accept, though)

> friend std::basic_ostream<charT, Traits>& 
> operator<< <>( std::basic_ostream<charT, Traits>& , 
>                const basic_format& ); 
> What does this syntax mean here? MSVC works without <>.

the '<>' means the operator we refer to is a template, with implicit
template parameters.
g++-3.0 gently asked me to add this, so I obeyed..
I think that without '<>' it would refer to the operator with the same
signature, but non-template.
for a given set of template parameters, we could indeed have the choice
between a template operator<<, and a non-template one (which is
preferred by the general resolution mechanism)
 
Do you mean MSVC requires to delete the '<>', 
or simply that it also works without ?

> template<class T>    void apply_manip( const T& manip) change type of 
> the argument from T& to const T&. Why was it not const? DEfinition is 
> moved in.

because some manipulators can not be passed by const-reference.

> all referenced of basic_string::insert( 0, ... ) substituted with 
> basic_string::insert( <basis_string instance>.begin(), ... ). 
> Ambiguity otherwise. This is beter anyway. The same for the 

g++ did not complain about ambiguities, but it should have..

> bench_variants.cc

I guess I'll simply launch it on my machine, put some results in a HTML
file, and remove the file from the archive.
portability and benchmarking does not mix very well.

> 1. Default allignment. The library seems to make some asumptions 
> about default ostream allignment( what in the standard?) Anyway it is 

when I made assumptions, I had the corresponding pages of the standard
right under the nose, so IMHO those assumptions are founded.

The standard is not very clear on alignements, but it does say how
things should be aligned by default in most cases.
In fact, I noticed a bug in gcc (2.95.* and 3.0.*) on the subject.
and I can imagine that many standard library implementations are faulty
on that point.

> different in STLPort implmentation. So you can imagine that most of 
> you tests/asserts are failing. I assume that format could try to 

yes, alignment problems were the toughest, so I wrote most of the tests
to check it was working okay.

> enforce desired default value itself ( I even tried to so by setting 

I'm not sure we should force the stream into 'the true
standard-compliant default state',
because the user of this stream library might expect the non-standard
default state, and be surprised by the behaviour of format.

> 2. Hex format. The library seems t omake some asumptions abould 
> default hex format., which is seems to be different on my 
> implementation. Moreover I sometimes getting 0X instead of 0x. Maybe 
> something wrong with hex handling flags?

0X should happen only when using 'uppercase' flags. (E, G, and X instead
of e, g, and x)

> 1. Code definitely need factoring.
>   a. Separate file for the details ( or even probably several, one 
> for utilities like str2num, one for stream_format_state and so on). 
> They could be in detail subdirectry.
>   b. Separate efile for exceptions. I may not need them at all, why 
> should I de[end on stdexcept?
>   c. format_fwd header. If I want my interface to contain 
> boost::format, should I depend on it implementation
>   d. format.h itself
>   e. implementation header

I've applied those suggestions on my local files, and the structure is
now :
"boost/format/format_fwd.hpp" 
"boost/format/format_internals.hpp"    
"boost/format/format_class.hpp"
"boost/format/format_exceptions.hpp"
"boost/format/format_implementation.hpp"
"boost/format/format_funcs.hpp"

all of which are included (in this order) by
"boost/format.hpp"

> 2. Coding style
> I do not think it is that important, but still why boost developed 
> coding standart if nobody even try to follow it? In format library I 
> found data members named like this:
>  cur_arg_
>  precision
>  argN
>  res

right.
inside my classes, data members names end with "_",
and when I designed small structs to hold a few variables together, I
did not feel the need for this kind of naming scheme.
I'll fix this, to have a uniform terminology.

> It better to be at least in one style. Or 3 neighbour member 
> functions like this:
> static bool parse_Sdirective
> static void do_fill( 
> static void skipAsterisk( 

agreed, and fixed locally
parse_Pdirective -> parse_printf_directive
parse_Sdirective -> parse_short_directive

> 3. Not enough comments.
> 
> If you will look onto implementation you see a lot of purely 
> commented huge blocks of code (see parse or parse_Pdirective for 
> examples).

You mean there are huge blocks of code that are within comments ?
I didn't find any, in parse or parse_Pdirective.

> It would be good for each function to have detailed 
> description what it is doing 
[...]
> I 
> really like stile proposed somewhere, like this:
> 
> // operator[] - short general description
> // Effect:
> //  do so and so ....
> //   ...
> // Returns: so and so
> // Throws: so and so

True, it can only clarify the code.

> template<class T>  basic_format&   operator[](const T& x)
> {
>     // 1. If dumped already them ...
>     // 2. Distribute supplied argument ...
>     // 3. If ... then for each ... do ...
>     // 4. Apply ... to ... 
> 
>     if(dumped_) clear();                     // 1 //
>     
>     distribute(false, x);                    // 2 //
>     ++cur_arg_;
>     if(bound_.size() != 0)                   // 3 //
>     {
>         while( cur_arg_ < num_args_ && bound_[cur_arg_] ) ++cur_arg_;
>     }
>       
>     state0_.apply_on(oss_);                  // 4 //
> 
>     return *this;
> }
>
> This is just an example. The main point is to merge all comments in 
> one place in a form of descriptive algorithm. This is espesially 
> importance in all nontrivial implementations which is format is 
> consist of. Details level is up to you.

Indeed it is quite nice.
But it depends on the context. This kind of comments suits this function
nicely.
But for the big and ugly parse_Pdirective, filled with details and
special cases, it's harder to merge all the step-by-step comments in one
place.
For this function, the 'algorithm' would either take 2 lines, 
or be as long as the function definition..

Thanks for your many suggestions
-- 
Samuel