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

...

Disclaimer: please consider my review as partial if at all because of

On Sunday, March 20, 2016 5:06 AM, Artyom Beilis wrote: 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(); 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