Re: [boost] [Boost-announce] [review][Fit] Review of Fit starts today : September 8 - September 17

21 Sep 2017

      On Thu, 2017-09-21 at 11:39 -0400, Lee Clagett via Boost wrote:
...
I accidentally sent this to boost-announce where it is blocked in
moderation - resending to the main list.
On Fri, 8 Sep 2017 07:02:27 -0400
Matt Calabrese via Boost via Boost-announce
 wrote:
...
A formal review of the Fit library developed by Paul Fultz II starts
today, September 8, and runs through September 17.
A branch has been made for the review. You can find Fit for review at
these links:
Source: https://github.com/pfultz2/Fit/tree/boost
Docs: http://pfultz2.github.io/Fit/doc/html/doc/
[...]
...
Requirements
This requires a C++11 compiler. There are no third-party dependencies.
This has been tested on clang 3.5-3.8, gcc 4.6-6.2, and Visual Studio
2015. Gcc 5.1 is not supported at all, however, gcc 5.4 is supported.
Contexpr support:
Both MSVC and gcc 4.6 have limited constexpr support due to many bugs
in the implementation of constexpr. However, constexpr initialization
of functions is supported when using the FIT_STATIC_FUNCTION and
FIT_STATIC_LAMBDA_FUNCTION constructs.
`FIT_LIFT` uses `auto` in a C++14-style lambda, and thus does not
compile when in C++11 mode.
Yes, but `FIT_LIFT_CLASS` does work in C++11. I will add a note about that.
...
...
Noexcept support:
On older compilers such as gcc 4.6 and gcc 4.7, noexcept is not used
due to many bugs in the implementation. Also, most compilers don’t
support deducing noexcept with member function pointers. Only newer
versions of gcc(4.9 and later) support this.
====================
Please provide in your review whatever information you think is
valuable to understand your final choice of ACCEPT or REJECT including
Fit as a Boost library. Please be explicit about your decision.
I would vote to accept the library.
Thanks for the review.
...
...
Some other questions you might want to consider answering:
 - What is your evaluation of the design?
Overall I have a positive impression of the design. In general there is
a logical consistency to the adaptors / decorators.
The `infix` operator still feels like a gimmick - why include it in the
library? I cannot think of an instance where I would use such a
utility, especially considering the fragility of operator precedence.
A lot of people think its useful. To me it seems to be useful over pipable at
removing parenthesis in the function call. So an infix `mfor`(ie the monadic
bind operator) could be used like this:

auto triples = view::iota(1) <mfor> [](int z) {
    return ints(1, z) <mfor> [=](int x) {
        return ints(x, z) <mfor> [=](int y) {
            return yield_if(x*x + y*y == z*z,
                    std::make_tuple(x, y, z));
        };
    };
};

Or if we get `=>` for transparent returns, theres even less balancing of
braces:

auto triples = 
    view::iota(1) <mfor> [](int z) =>
    ints(1, z) <mfor> [=](int x) =>
    ints(x, z) <mfor> [=](int y) =>
    yield_if(x*x + y*y == z*z, std::make_tuple(x, y, z));

Of course, builtin support for monadic comprehension in C++ would be better,
but this is probably the best you can do without resorting to macros.
...
...
 - What is your evaluation of the implementation?
I think there are too many macros internally, but I think its mostly to
work around compilers. So its about average for a boost library.
Well macros are used to help improve compatibility, and improve compile-time
performance where possible(ie using intrinsics over type traits).
...
I also
do not like the inheritance from user-callables, as it seems
unnecessary in some cases (although perhaps easiest for `match`, etc.?).
Of course, if a function is not meant to inherit, it should be declared with
`final`, but if thats not enough, you can use `capture(f)(apply)` to prevent
inheritance.
...
...
 - What is your evaluation of the documentation?
The documentation seems to be improved from what I remember. At first I
felt like the getting started and examples section was "too much too
fast", but I am not sure there is an easy way to introduce a programmer
to some of these concepts. Is the library inspired by another, or
entirely by "point-free programming"? I am trying to think of some
outside resource to reference, but I cannot think of any.
It heavily-inspired by the Boost.Egg library:

http://p-stade.sourceforge.net/boost/libs/egg/doc/html/index.html

Which there is a link to in the 'Acknowledgements' section.
...
...
 - What is your evaluation of the potential usefulness of the library?
Some of the utilities will be valuable for C++11 projects (move
capture), and I also like the "pack" handling functions that will not
be available until C++17.
Are you referring to structured bindings?
...
A number of the adaptors seem like they could
be useful, but I've also not written anything similar or considered
doing so. Which might mean I need to do more functional programming.
...
 - Did you try to use the library? With which compiler(s)? Did you
   have any problems?
This time I just used the `BOOST_LIFT` function, tried out the
auto-placeholder and pipable adaptor to study their behavior.
...
 - How much effort did you put into your evaluation? A glance? A quick
   reading? In-depth study?
I spent significantly more the first review, particularly with the
implementation. I spent only a 1-2 hours looking at the code /
documentation this time.
...
 - Are you knowledgeable about the problem domain?
This is a fairly unique project; I do not know specifics about this
type of point-free or functional programming. I am fairly familiar with
std/boost bind and the usage of basic callables.
...
 - Were the concerns from the March 2016 review of Fit addressed?
I brought up inheritance from user types as a potential issue. The
author disagreed? A small implementation quirk, but can lead to
surprising behavior with operator overloads, etc.
I also suggested (i.e. not a concern necessarily) that the placeholders
be placed into their own namespace. This would allow
`using boost::fit::placeholders` without bringing the entire
`boost::fit` namespace into scope. The library could still alias the
placeholders into the `boost::fit` namespace as well.
That is a good idea. I will do that.
...
There is also this strange oddity:
    struct {
        int operator()(int x, int y = 10) const noexcept {
            return x + y;
        }
    } sum{};
    const auto pipe_sum = boost::fit::pipable(sum);
    int main() {
  std::cout << (1 | pipe_sum) << std::endl;
  return 0;
    }
The function is immediately evaluated, without `()` on pipe_sum and
outputs `11`. I looked through my post last time, and recommended that
something in the documentation mention that `pipable` behaves like a
`partial` evaluation.
There is a discussion about partial evaluation and its pitfall, here:

http://pfultz2.github.io/Fit/doc/html/doc/src/partialfunctions.html

I do mention that pipable is one of the adaptors that uses partial evaluation,
but I probably should mention it on the pipable reference page.
...
Or again, maybe that's obvious from the pack
listed in the documentation? So while the partial evaluation might be
obvious due to the documentation, the immediate evaluation without the
extra `()` is still intriguing.
I don't mention that. This probably should be mentioned and added to the
semantics section.

Paul

.

Re: [boost] [Boost-announce] [review][Fit] Review of Fit starts today : September 8 - September 17

paul