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: paul (pfultz2_at_[hidden])
Date: 2017-09-21 19:23:42

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

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

.