[boost] [fit] Mini Review (consider 1/2 or 1/4 weight)

Subject: [boost] [fit] Mini Review (consider 1/2 or 1/4 weight)
From: Artyom Beilis (artyom.beilis_at_[hidden])
Date: 2016-03-20 06:05:59

Disclaimer: please consider my review as partial if at all because of
the way I did it (see notes below)

> - Whether you believe the library should be accepted into Boost

No, I don't think so.

I don't feel that library adds any real value to boost: being useful
or in other words
significantly simplifying rather straight forward tasks can't be done

However I understand that there are some cases with template meta-programming
that can make library useful shown there:
http://article.gmane.org/gmane.comp.lib.boost.devel/265453

So I do think it another major review after significant redesign can be done.

> - Your name

Artyom Beilis

> - Your knowledge of the problem domain.

I worked a lot with lambda expressions and various callbacks
also mostly in dynamic manner rather implementing functional
programming.

> - What is your evaluation of the library's:
>   * Design

I did very brief analysis of design. What concerns me
is wide use of static objects and declaration of "static functions".

There is significant difference between

static struct foo_class {
   void operator()() const;...
} foo;

and

inline void foo()
{
}

Of course two issues:

1. Inline functions are weak symbols and don't violate ODR
2. Static variables in C++ do violate ODR (btw in C they do not) so
basically you COPY every instance of the functions across compilation
unit.

If you define some static function that does something  by macro I'd
rather expect to see stuff like

inline void foo(...)
{
    static foo_instance ... f = ...;
    f(...)
}

I don't know if it is implementable but it makes more scene or at
least justify use of macro.

On the other hand I don't see (with exception of some corner cases)
why this is better than

BOOST_FIT_STATIC_FUNCTION(foo) = fit:compose(bar1,bar2)

// * calls bar1(bar2(...))
inline void foo(...)
{
    bar1(bar2(...))
}

or inlined

auto foo =  fit:compose(bar1,bar2);

Is better than

auto foo = [](...) { bar1(bar2(...) ) };

I understand (after getting this reply
http://article.gmane.org/gmane.comp.lib.boost.devel/265453)

That there are some meta-programming cases that the library is really helpful.
But this isn't something I see in library documentation or it isn't
the major library purpose.

And for trivial cases I think direct approach is better as it is just
clearer and easier to
understand for an average code maintainer.

>   * Implementation

Reviewed barely... no comment

>   * Documentation

This is another major issue it does not clear what library does and
how it is useful.
This alone would make a condition on the acceptance.

>   * Tests

Didn't reviewed

>   * Usefulness

It is probably the major questions that the library need to ask.

What is the purpose of the library and what problem it solves. I
didn't find a complete
answer to this question.

> - Did you attempt to use the library? If so:

No

> - How much effort did you put into your evaluation of the review?

Several hours - mostly reading docs and code.

I can't say I reviewed most of the code or most of the docs. I stopped
before as I couldn't find the satisfying answers regarding what
library does.

I understand that if somebody would submit the library regarding astrophysics
or bio-modeling I'd had hard time to understand what library does.
However regarding FIT I don't think it is the case.

So if you (review manager/library designer) feel that I attempted to
review a library
in a field I don't get - feel free to ignore my review.

Artyom Beilis