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

Subject: Re: [boost] [Boost-announce] [review][Fit] Review of Fit starts today : September 8 - September 17
From: Lee Clagett (forum_at_[hidden])
Date: 2017-09-21 15:39:22

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
<boost-announce_at_[hidden]> 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.

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

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

>  - 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. I also
do not like the inheritance from user-callables, as it seems
unnecessary in some cases (although perhaps easiest for `match`, etc.?).

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

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

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

> More information about the Boost Formal Review Process can be found
> here: http://www.boost.org/community/reviews.html
> 
> Thank you for participating!
> 

Lee