[fit] Mini Review (consider 1/2 or 1/4 weight)
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
I want to make a minor remark about this:
[...]
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.
[...]
FIT (and range-v3, and many other libraries) follow the best practice we currently have in C++ < 17 here, which is Eric Niebler's N4381 Suggested Design for Customization Points [0]. It seems, however, that from the discussion here and in the other reviews of FIT, this best practice is not "common practice" yet. The failure is only on us for not giving it enough exposure. IMO all new Boost libraries should be following N4381 as FIT does. On first encounter, however, a macro like BOOST_FIT_STATIC_LAMBDA is going to look very suspicious: why is this required? what about ODR? ... Since these macros are going to remain the only viable solution for a long time (until we drop support for C++<17) we should address these questions on first usage in the documentation of new libraries, maybe by linking against N4381 directly (which addresses the issue in great detail) or by providing a small summary of N4381 in the documentation or the Boost wiki. [0] http://ericniebler.github.io/std/wg21/D4381.html#no-violations-of-the-one-de...
On Sun, Mar 20, 2016 at 7:53 AM, Gonzalo BG <gonzalobg88@gmail.com> wrote:
I want to make a minor remark about this:
[...]
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.
[...]
FIT (and range-v3, and many other libraries) follow the best practice we currently have in C++ < 17 here, which is Eric Niebler's N4381 Suggested Design for Customization Points [0].
It seems, however, that from the discussion here and in the other reviews of FIT, this best practice is not "common practice" yet.
The failure is only on us for not giving it enough exposure.
IMO all new Boost libraries should be following N4381 as FIT does.
On first encounter, however, a macro like BOOST_FIT_STATIC_LAMBDA is going to look very suspicious: why is this required? what about ODR? ...
Since these macros are going to remain the only viable solution for a long time (until we drop support for C++<17) we should address these questions on first usage in the documentation of new libraries, maybe by linking against N4381 directly (which addresses the issue in great detail) or by providing a small summary of N4381 in the documentation or the Boost wiki.
[0]
http://ericniebler.github.io/std/wg21/D4381.html#no-violations-of-the-one-de...
+1
Le 20/03/2016 21:55, Zach Laine a écrit :
On Sun, Mar 20, 2016 at 7:53 AM, Gonzalo BG <gonzalobg88@gmail.com> wrote:
I want to make a minor remark about this:
[...]
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.
[...] FIT (and range-v3, and many other libraries) follow the best practice we currently have in C++ < 17 here, which is Eric Niebler's N4381 Suggested Design for Customization Points [0].
It seems, however, that from the discussion here and in the other reviews of FIT, this best practice is not "common practice" yet.
The failure is only on us for not giving it enough exposure.
IMO all new Boost libraries should be following N4381 as FIT does.
On first encounter, however, a macro like BOOST_FIT_STATIC_LAMBDA is going to look very suspicious: why is this required? what about ODR? ...
Since these macros are going to remain the only viable solution for a long time (until we drop support for C++<17) we should address these questions on first usage in the documentation of new libraries, maybe by linking against N4381 directly (which addresses the issue in great detail) or by providing a small summary of N4381 in the documentation or the Boost wiki.
[0]
http://ericniebler.github.io/std/wg21/D4381.html#no-violations-of-the-one-de...
+1
+1 Vicente
On Sunday, March 20, 2016 8:11 AM, Gonzalo BG <gonzalobg88@gmail.com> wrote:
I want to make a minor remark about this:
[...]
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.
[...]
FIT (and range-v3, and many other libraries) follow the best practice we currently have in C++ < 17 here, which is Eric Niebler's N4381 Suggested Design for Customization Points [0].
It seems, however, that from the discussion here and in the other reviews of FIT, this best practice is not "common practice" yet.
The failure is only on us for not giving it enough exposure.
IMO all new Boost libraries should be following N4381 as FIT does.
On first encounter, however, a macro like BOOST_FIT_STATIC_LAMBDA is going to look very suspicious: why is this required? what about ODR? ...
Since these macros are going to remain the only viable solution for a long time (until we drop support for C++<17) we should address these questions on first usage in the documentation of new libraries, maybe by linking against N4381 directly (which addresses the issue in great detail) or by providing a small summary of N4381 in the documentation or the
Boost wiki.
I agree and plan to add this to the documentation.
[0] http://ericniebler.github.io/std/wg21/D4381.html#no-violations-of-the-one-de...
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Disclaimer: please consider my review as partial if at all because of
On Sunday, March 20, 2016 5:06 AM, Artyom Beilis <artyom.beilis@gmail.com> 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<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
On 21/03/2016 02:11, paul Fultz wrote:
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?
On that topic, one thing that really puzzled me as I was browsing the library docs is that there are little to no motivating examples in the reference section. This may be my own fault, but I'm accustomed to be able to jump straight to the reference docs for any particular class/function (without reading much more than the basic overview of the library as a whole) and see information on what it does and why I would want to use it. Some of the latter are indeed provided elsewhere (such as the page you linked above), but it would be nice if the reference pages were either more fully fleshed out or at least included links back to the corresponding examples.
On Sunday, March 20, 2016 at 6:47:57 PM UTC-5, Gavin Lambert wrote:
On 21/03/2016 02:11, paul Fultz wrote:
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?
On that topic, one thing that really puzzled me as I was browsing the library docs is that there are little to no motivating examples in the reference section.
This may be my own fault, but I'm accustomed to be able to jump straight to the reference docs for any particular class/function (without reading much more than the basic overview of the library as a whole) and see information on what it does and why I would want to use it.
Some of the latter are indeed provided elsewhere (such as the page you linked above), but it would be nice if the reference pages were either more fully fleshed out or at least included links back to the corresponding examples.
I think it would be a good idea to add a "see also" section to cross reference larger examples in the documentation.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Thanks Artyom for your review. I believe you have missed the goal of the library, but this re-enforce the idea that the library needs better document its goal. I will take it into consideration as all the comments exchanged during these 17 days. Best, Vicente Le 20/03/2016 11:05, Artyom Beilis a écrit :
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
participants (7)
-
Artyom Beilis -
Gavin Lambert -
Gonzalo BG -
paul Fultz -
Paul Fultz II
-
Vicente J. Botet Escriba -
Zach Laine