[boost] FC++: formal review

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2004-02-28 14:53:23

Hi,

   It was quite difficult to review this submission for several reasons:
mostly because (from POV)  it has virtually no documentation, no structure,
no testing and what is most important no clear defined problem domain, where
this library provide a solution (here I mean not the function objects
facilities, but namely FP part of the submission). What it does have, is a
several interesting ideas. In summary it my opinion this submission is
unacceptable and my vote is NO.

Problem domain
-------------------------------------------------------------------------
   Unlike my usual review I would like to start with problem domain
discussion. Library position itself as a library to do FP in C++. Namely in
this order. Kind of like library for crazy Haskell programmer that for
whatever reason decided to use C++. Docs (and papers) in details describe
how to do things one could do in Haskell using FPC++ analogs. There is even
comparison of performance with Haskell compilers: not to worry we almost as
fast. For me as a C++ programmer more important is to see the comparison
with the same algorithm implemented with other usual C++ techniques. I did
one myself. I took an example used by authors for performance testing and
modified it like this:

.... here is what is already in test

////////////////////////////////////////////////

inline bool is_prime( int i )
{
    for( int c = 2; c <= i/2; ++c ) {
        if( i % c == 0 )
            return false;
    }

    return true;
}

inline int nth_prime( int N )
{
    int i = 1;
    while( N > 0 ) {
        i++;

        if( is_prime( i ) )
            N--;
    }

    return i;
}

int main() {
   acc_timer timer; // acc_timer is my timer class based on gettimeoftheday

   int NUM;

   cout << "Num primes = ";
   cin  >> NUM;

   timer.start();

   list<int> l = primes(NUM);
   int i = at( l, NUM-1 );

   timer.stop();

   cout << i << endl << "took " << timer.elapsed() << " sec" << endl;

   timer.start();

   i = nth_prime( NUM );

   timer.stop();

   cout << i << endl << "took " << timer.elapsed() << " sec" << endl;
}

The algorithm I am using similar to the one use by original code. The only
difference is that I am not using FC++ abstraction. I was using gcc 3.3.1
under cygwin on my Athlon 2000+ PC. I was using following command for
compilation:
g++ -O4 -I [list of include directories] tw_primes.cpp

Here is the result of several runs:

Num primes = 500
3571
took 4.825 sec
3571
took 0.015 sec

Num primes = 750
5693
took 11.703 sec
5693
took 0.036 sec

  In average FC++ solution is 320 times slower. For any practical usage
price of an abstraction should not exceed 10-20%. With price as high as FC++
currently shows I wouldn't even start to think in this direction.
  In general from my experience usage of lazy evaluation quite limited (in
FP sense - I do not mean binding the arguments, which is widespread). It may
be used sometimes, but I know many more other neat tricks that are more or
as useful. IMO authors need to justify need in lazy list as a generic
reusable components in C++ (namely in C++ not for FP which says nothing to
me), but for now I do not see a place for them. Some papers does try to
justify need for FP to implement common design patters in C++. But actually
they only shows need for use of polymorphic functors nothing more.

Design
-------------------------------------------------------------------------
  I with almost everything authors says about need of polymorphic functors
and the way they design support for result type deduction. But I believe it
should be boost-wide solution. With support in both boost::function,
boost::bind and boost::lambda.
  I don't think we need indirect functoids (with their virtual functions and
smart_pointers inside). boost::function should be made result_of aware
instead.
  I couldn't comment on design of lazy lists since it does not described in
docs. But I still not convinced that there exist a problem to justify need
for this construct.
  Lambda facilities design is also offsite and it's difficult to comment on
it by itself and on comparison with boost::lambda. In my personal opinion
there should not be two different facilities dedicated to the same cause
within boost. If you have any issues with boost::lambda lets resolve them.
In other case it would only source of confusion to the users and make them
both less useful.
  Monads are not covered and not commented accordingly.
  Explicit built-in currying (I prefer term binding) looks like convenient
idea and I would vote to include this info boost::function
  Implicit built-in currying on the other hand like more like a source of
confusion than convenience and in my opinion should not be included. It not
only may confuse inexperienced user, but also may cause a conflict If I have
function polymorphic not only in regards to the type of the arguments but
also in regards to their number.
   I do not understand need for separate notion of thunks. Why couldn't we
just bind all function arguments using usual means?
   Library supply the collection of predefined function object. While I can
blindly accept polymorphic counterparts to the STL functional ones, the rest
should be documented and discussed one by one (the fact they belong to
mythical "prelude" says nothing to me). Also from sources it looks like many
of them operate on lazy lists. Why couldn't we have them as a member
functions? They are not generic to be used for different kinds of lists
(like STL list) and what is an advantage to length ( ll ) vs. ll.length().
Would I need to pass this method anywhere (rare case IMO) I could use an
adaptor.
  Infix syntax: I do not think this feature useful enough to justify weird
syntax. Most probably it wouldn't be use for anything other then several
functors like plus, minus.

Documentation
-------------------------------------------------------------------------
  There two major problems I have with docs: first they are very scarce on
design of the  component described and second they are written for Haskell
programmer who is learning C++. I believe that at least 90% of C++
practitioners never heard about Haskell. Accordingly it should not be
mention in docs AT ALL. The should not be ANY references to Haskell syntax,
which instead of clarifying things make it worse and left me with feeling
that I did not get something. There should be a short introduction to what
is a functional programming paradigm. Comparison with existent C++
paradigms, advantages and disadvantages. I don't say it should be extensive
multipage work, but just an introduction for C++ programmer not familiar
with FP.
  There should be reference documentation for any functor supplied by the
library. I am not sure there is a need for old (pre boost) references in
docs. It absolutely unnecessary for new users. Why existent users may use
some kind of transition guide in FC++ website.
  The rest of docs looks in most part as a "simple overview", while I expect
to see a real full documentation.

Implementation
-------------------------------------------------------------------------
0. It would be much easier if you would follow recommended boost directory
structure
1. BOOST_FCPP_DEFER_DEFINITIONS
   This does not look like a good solution. What exactly did you try to
achieve?
2. definition.cpp
    What is this? Does it gets compiled by any compiler?
3. All implementations needs to be rewritten using Boost PP to be extendable
to more then 3 arguments.
4. Why fullN does not inherit from it's parameter, but keep it as a member
instead?
5 What is konst vs. const?
6. When you will eliminate implicit currying implementation became more
simple
7. binderNandK...ofM facility does not look extendable. Did you look how
boost::bind is implemented?
8. There are still a lot of identifiers that does not follow recommended
boost naming policy
9. Why infix syntax implementation is mixed with some library supplied
functors?

Testing/Examples
-------------------------------------------------------------------------
There is a lot of staff in fcpp_clients directory. Since there is no
test/examples separation and no Jamfiles I couldn't comment in details how
it looks.

Regards,

Gennadiy.