Boost logo

Boost :

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.


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk