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

Subject: Re: [boost] [fit] Mini Review (consider 1/2 or 1/4 weight)
From: paul Fultz (pfultz2_at_[hidden])
Date: 2016-03-20 09:11:00

> On Sunday, March 20, 2016 5:06 AM, Artyom Beilis <artyom.beilis_at_[hidden]> wrote:
> > 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.

Thats what the macro does. It declares the variable at class scope. The macro
does the equivalent of this:

template<class T>
struct static_const_storage
{
    static constexpr T value = T();
};

template<class T>
constexpr T static_const_storage<T>::value;

template<class T>
constexpr const T& static_const_var()
{
    return detail::static_const_storage<T>::value;
}

static constexpr auto& foo = static_const_var<foo_class>();

In addition there are tests here:

https://github.com/pfultz2/Fit/blob/boost/test/static_def/static_def.cpp#L53

Which check that the object has the same address across translation units.
This means that there is no ODR violation and no executable bloat due to
excessive copies across translation units. Also, in C++17, this will be
replaced by inline variables:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4424.pdf

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

Those examples are documented here:

http://pfultz2.github.io/Fit/doc/html/more_examples/index.html

What other cases do you see missing in the 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
> 
> _______________________________________________
> Unsubscribe & other changes: 
> http://lists.boost.org/mailman/listinfo.cgi/boost
>