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

From: rogeeff (rogeeff_at_[hidden])
Date: 2002-01-21 03:55:18

Hi,

PART 1. MSVC compilation and testing

I was able to compile format library unver MSVC6SP5 with STLPort 4.5. 
I was forced to  make several modifications to the  code to do so. 
One major one is since MSVC does not recognize template member 
function defined outside template class definition, they need to be 
moved from implementation header to the main header. Another thing is 
that MSVC does not recognize neither .cc extention as valid C++ 
source file (I do know how to overcome this, but this is still an 
issue ), nor .ihh as valid C++ header file; what make life of any 
MSVC developer that trying to use format more difficult. Why not to 
use .cpp and .hpp? ( What boost coding says about it?)

In details (based on version by 1/20/02):

format.hpp:

template<class charT, class Tr>
int str2int(const std::basic_string<charT, Tr>& s, size_t i=0) 
definition is moved in. Though it is not a template member function, 
MSVC fails to work otherwise.

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

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.

template<class T> bind_arg(int argN, const T& val) definition is 
moved in.

template<class T> set_flag(int itemN, const T& manipulator) 
definition is moved in.

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

template<class T> void distribute(bool is_manip, const T& x) 
definition is moved in.
template<class T> void distribute(bool is_manip, T& x) declaration is 
completely removed.

template<class T> void put(const T& x, const format_item& specs, 
string_t & res) definition is moved in. 
template<class T> void put(T& x, const format_item& specs, string_t & 
res) declaration is completely removed.

static bool parse_Pdirective(const string_t & buf, 
                             typename string_t::size_type* pos_p, 
                             format_item* fpar) definition is moved 
in. Though it is not a template member function, MSVC fails to work 
otherwise.

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.

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

format.ihh

all template member function definition are deleted.
~275 line MSVC for scope bug.

all_test/sample files:

str dunstituted with boost::format::str. Unresolved otherwise. Seems 
like MSVC bug.

bench_variants.cc

Your "portable" /dev/null stream equivalent did not work for me. Here 
what I did:

using std::ostream;

class NulStream : public ostream 
{
public:
  NulStream();
  virtual ~NulStream();
  NulStreambuf*    rdbuf() {
    return static_cast< NulStreambuf* >( ostream::rdbuf() ) ;
  }
} ;
 
NulStream::NulStream()
: ostream( new NulStreambuf )
{
}

MSVC does not have Unix98 compartible snprintf. _snprintf could be 
used instead but you won't get a desired result string.

100000 seems to be enormous value for me to wait, but I was able to 
perform couple attempt with 5000.

That is all with compilation. BUT! There are two more very important 
issues:

1. Default allignment. The library seems to make some asumptions 
about default ostream allignment( what in the standard?) Anyway it is 
different in STLPort implmentation. So you can imagine that most of 
you tests/asserts are failing. I assume that format could try to 
enforce desired default value itself ( I even tried to so by setting 
flag  value in the stream_format_state<charT,Tr> ::reset() method. 
But it appears that this methods *never* referenced. Why does it 
exist? I then tryies to place call reset() into stream_format_state 
constructor but this did not help)

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?

Performance comparison gave me following results (since snprintf is 
not compartible no reason to compare with snprintf version):

Release mode:
ostream         1
stored format   ~ 4.2 in avarage
format          ~ 13.1 in avarage

Debug mode
ostream         1
stored format   ~ 3.7 in avarage
format          ~ 9.1 in avarage

What interesting in release mode relation even worse.

PART 2. Code comments:

Here is my major concern about this library. I can'y give in-depth 
code review since I was not able to get through it (Everything after 
that is IMHO). Boost should present examples of well-structures, well-
documented, easy to read and understand code, which is what this code 
in many places fail to comply to. Here what I can say :

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

Again it's better to stick to one stile (And what is Sdirective?).

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). There are some comments like this:

case 'h': break;  // short or long modifier.. we dont need to know !

But this does not produces a general picture. It is very difficulf to 
follow what is going on there, especialy with presence of those goto 
statements. It would be good for each function to have detailed 
description what it is doing ( not like this:

 // system for binding arguments :
 template<class T> basic_format& bind_arg(int argN, const T& val)

}

And then insede the code step by step desription what is going on. 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

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.

PART 3. Samples/Testing

TEst presented does not seems to cover all the funtionality presented 
by the format library, including all exception that could be thrown 
(New Test Library would help here). On the other side samples seems 
to be too complex, or even not a samples at all, but rather another 
tests for some reason implemented using asserts (maybe only 
sample_userType.cpp will qualify as an example). It would be better 
to present series of tiny simple examples each cover one part of 
format functionality.

PART 4. Conclusion

I think that boost::format library present very valuable 
functionality and in most do the job it is advertised to do.Though I 
would like to see some of my concerns to be addressed before 
accepting the library I do not think that they will be show stoppers.

Regards,
Gennadiy.