________________________________________ From: P F [pfultz2@yahoo.com] Sent: 08 September 2017 14:06 To: boost@lists.boost.org Cc: Fletcher, John P Subject: Re: [boost] [review][Fit] Review of Fit starts today : September 8 - September 17

...

Is that detailed report available somewhere please?

No, a detailed report was never published.

Oh well. I have just started to look at this in comparison with some examples I made with a version downloaded in February 2017. I have not found any list of the changes. I have noted the following. 1. Change namespace to boost::fit 2. Relocation of header files 3. Add BOOST_ to macro names. All of those I expected. Also two of the examples I had done no longer work as "compress" and "reverse_compress" have disappeared. Have they been renamed to something else or removed completely? Are there any other changes please? Best wishes John

Le 08/09/2017 à 14:04, Fletcher, John P via Boost a écrit :

________________________________________ From: Boost [boost-bounces@lists.boost.org] on behalf of Matt Calabrese via Boost [boost@lists.boost.org] Sent: 08 September 2017 12:02 To: Boost Developers List Cc: Matt Calabrese Subject: [boost] [review][Fit] Review of Fit starts today : September 8 - September 17

...

A formal review of the Fit library developed by Paul Fultz II starts today, September 8, and runs through September 17. .....

...

This is the second time that Fit will go through the review process, with the previous time being in March of 2016. The result of the initial review can be found here: In this second review, those who choose to participate should be as rigorous as they'd be in any other review, but are also encouraged to voice whether or not the concerns from March of 2016 were addressed. I looked at the previous review and found the following:

----------------------------------------------- Detailed Report ===============

You will need to wait yet for a month before I will be able to do it. On the mean time there are a lot of issues that must be addressed and that are part of the github issues.

------------------------------------------------

Is that detailed report available somewhere please?

Hi, I'm really sorry. I didn't take the time to write such a report :( My apologies. Vicente

On Mon, 2017-09-11 at 11:55 -0500, Zach Laine via Boost wrote:

On Fri, Sep 8, 2017 at 6:02 AM, Matt Calabrese via Boost < boost@lists.boost.org> 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/

[snip]

...

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 vote to accept Fit.  I would like to see some naming changes, but I'm not making any of them conditions for acceptance, because I do not consider any of them to be deal-breakers.

Thanks, Zach, for the review.

Some other questions you might want to consider answering:

...

  - What is your evaluation of the design?

(Still) good.  Here's what I wrote in the last review:

I particularly like the fact that it attempts to keep everything constexpr- and SFINAE-friendly wherever possible.

I *really* like reveal() and failure(). Too few TMP-heavy libraries pay attention to providing good error messages.

I have no problem with the constness requirements on Callables used throughout Fit, as was discussed previously. I even consider this a feature, allowing me to detect non-const Callables, which are almost always an error on my part. This is especially true since std::ref() and mutable_() let me explicitly choose how to handle mutable Callables, in a way that is also explicit at the call site.

There are a number of adaptors that I find have obscure names:

by() is a projection; why not call it project()?

by() is not a projection, rather it takes a projection. I mainly named it for how it was used, so it would read more english-like: std::sort(std::begin(people), std::end(people),         by(&Person::year_of_birth, _ < _)); Like "sort by year_of_birth". In haskell, its called 'on', but it seems 'by' is more natural for English.

flow() is a super obscure name! Why is this not reverse_compose() or similar?

Its because its used with pipable. So it can take a set of pipable functions so they "flow" together. reverse_compose() could work, but I think the name looks clunky as an alternative to pipable functions.  Also, in haskell there is no reverse compose(just `.` for composition), instead the `>>>` arrow operator can be used to indicate the direction of composition, but there is no way to implement such an operator in C++.

indirect() could more easily be understood if it were called deref() or dereference().

This named after the indirect in the Ranges library. Also, I might add a custom operator in the future, so calling it deref might not make sense.

I don't feel as strongly about partial(), but I think it might be clearer if it were called partial_apply() or curry().

Which there could be an uncurry function, but I've always understood curry to be for one arg at a time. Either waty, I could provide curry for repeated partial applications(ie what partial is currently). Then provide partial and reverse_partial like hana for a single partial application. I just wonder if this bloats the interface, as we will have a lot of different functions for partial application.

conditional() should be called invoke_first(), call_first_of(), or similar.  I find it too easy to confuse with if_().

I could call it first_of(). I do see the confusion with `std::conditional`.

For that matter, it would be nice if "if_()" were called "if_constexpr()", since that seems to match its semantics, and since there's a language feature that already does this; a reader of uses of "if_constexpr()" will get the intent faster than they would if they read "if_()".

To work like if_constexpr(), eval() would needed to be used. There are of course other ways to use it(especially in point-free programming) that is not an emulation of `if constexpr`, like how I implement `yield_if` here: https://github.com/pfultz2/Hero/blob/master/include/hero/for_each.h#L43

...

  - What is your evaluation of the implementation?

I did not look at it much, but what I did look at had no readily apparent issues.

...

  - What is your evaluation of the documentation?

It has gotten dramatically better since the last review.  The non-reference sections now provide a thorough tour of the library and its capabilities.

I took issue with some of the reference docs in the last review; those have all been addressed.

...

  - What is your evaluation of the potential usefulness of the library?

I think the library is very useful.  For example, I had to do some somewhat hairy overload-resolution metaprogramming in an expression template library I've been working on, and I really wished conditional() were already in Boost so I could have used it instead.

Well, hopefully it will be there soon. Thanks, Paul

On Sep 13, 2017, at 7:47 PM, Edward Diener via Boost wrote:

On 9/8/2017 7:02 AM, Matt Calabrese via Boost 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.

And that branch is … ?

Its the branch boost. Thats what the source links to.

On Fri, 2017-09-15 at 10:48 +0000, Fletcher, John P via Boost wrote:

Here is my review of the FIT library

...

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 vote to ACCEPT  FIT as a Boost library.

Thanks, for the review.

...

Some other questions you might want to consider answering:  - What is your evaluation of the design?  - What is your evaluation of the implementation?

I have found both the design and the implementation to be consistent and well carried through. There are a few things which are in the detail which are in fact of use to users and it would be helpful it they were available in the fit namespace. See notes below.

...

 - What is your evaluation of the documentation?

The general standard of the reference documentation is good. It would help to have some more examples.

What kind of more examples would you like to see?

There are some things which are not documented such as alias and the configuration.  Also move and forward which are in detail although useful to users.

  - What is your evaluation of the potential usefulness of the library?

I think this is a very useful library.  I have explored this by building an example use - see notes below.

  - Did you try to use the library? With which compiler(s)? Did you     have any problems?

Yes, I have built examples to run everything I can find in the library.  I did this with the version available in February this year and have moved the examples over to the Boost version with the necessary changes to namespace and boostification of the macros.  All worked after I found the name changes to compress/reverse compress to fold/reverse_fold.  I see that this is a response to previous discussion and I agree with the changes.

  - How much effort did you put into your evaluation? A glance? A quick     reading? In-depth study?

I have been working with FIT for some time and building the examples.  I have mainly used various versions of clang and libc++, also using various versions of gcc. I have discovered that not all of the compiler configuration is carried out in fit/configure.hpp.  The following headers also detect the compiler being used:  alias, always, implicit, pack, reveal, unpack.

Some of those is because there is no specific compiler feature it is working around. Although in a few places I could use another config.

I have built examples of all of the code I can find, including for alias where there is no documentation and only test examples for some of the things in the header.  It is used in pack and combine and may have been intended to be internal although it can be used externally.

It is intended to be used externally, which is why there is an initial documentation, and it is not in the detail namespace. Many didn't see how it is related to this library, plus it was missing a useful example. So for now, I just took it out of the TOC, but would like to add it back, but need a good motivating example. Thanks, Paul

On 9/15/17 11:45 AM, Robert Ramey via Boost wrote: I much like the "Getting Started" page- it's really helpful and clearly presented. So maybe my critcism came off harsher than it should be. Robert Ramey

On 15. Sep 2017, at 20:45, Robert Ramey via Boost wrote:

I don't like names like Fit which give no helpful information. But I've lost that battle before so I won't belabor this any more.

I would like to join the chorus of people who don't like the name "Fit". Please - please - consider changing this. "Fit", what does it mean? Is it an acronym (perhaps for function interface toolkit??)? Was the library written in a "fit" of rage over the lack of proper tools? Is the library in particular good health, perhaps? The documentation does not tell us, but even if it would, a longer, more descriptive name would be helpful. In my personal professional context, a "fit" refers to code that adapts a statistical model to stochastic data by minimising a cost function, which somehow measures the closeness of the model and the data. Googling "c++ fit library" yields links to Boost.Fit, but the first other hits are related to such optimisation libraries (this is not because of my personal Google search bubble, I used a private browser session). This as another hint that the name "Fit" is misleading. If I may suggest a name: I like description on github "function utility library". So what about "Function Utilities", which is quite descriptive. On the Boost library page it would be inserted next to "Function" and "Function Types", which - ironically - seems quite "fit"ting. PS: Googling "fit library" yields such curious results as "Fashion Institute of Technology" in NYC and the "Florida Institute of Technology" (both have a library). :) Best regards, Hans

On 9/18/17 7:45 AM, paul via Boost wrote:

...

If I may suggest a name: I like description on github "function utility library". So what about "Function Utilities", which is quite descriptive. On the Boost library page it would be inserted next to "Function" and "Function Types", which - ironically - seems quite "fit"ting.

But if I use the FunctionUtilities, I would find calling `boost::function_utilities::pipable` or `BOOST_FUNCTION_UTILITIES_STATIC_FUNCTION` to be just too long.

at least as far as the namespace is concerned, users can and often do declare namespace alias. Users could also use a forwarding define - eg #define BFU_STATIC_FUNCTION BOOST_FUNCTION_UTILITIES_STATIC_FUNCTION So if the length of names is a concern it is addressable. Perhaps a small section in the docs might suggest this. There's no issue with name collision as it's it totally in the hands of the user who might want to make his own code more readable. Speaking of docs - a section FAQ might be a good place for all the miscellaneous questions which arise in the review and subsequent complaints. If you keep updating this, you'll find that you get no more questions eventually. FunctionUtilities. This seems to overlap with boost.callable library. here are ones that occurred to me UnifiedFunctionCalling UnifiedFunctionInteraces FunctionInterfaces Naming is always an issue and never satisfies everyone. Just do your best. Robert Ramey

I think HigherOrderFunctions is much more descriptive, and then I could use the namespace `hof` for short.

Also, the word 'utility' is not very descriptive, and I would prefer to move away from using it. Boost.Utility contains random things in it that are unrelated, and I don't want this library to be described as a collection of random and unrelated functions.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On 9/18/2017 10:45 AM, Hans Dembinski via Boost wrote:

...

On 18. Sep 2017, at 16:45, paul wrote:

But if I use the FunctionUtilities, I would find calling `boost::function_utilities::pipable` or `BOOST_FUNCTION_UTILITIES_STATIC_FUNCTION` to be just too long.

I think HigherOrderFunctions is much more descriptive, and then I could use the namespace `hof` for short.

I hope I am not bike-shedding here. I do believe that names are important, because they are the first thing you see of a library, and I believe that "obviousness" is an important factor.

I can see where you are coming from, but let me try to defend the name "Function Utilities" a bit, because although I think that using another abbreviation like "hof" is better than "fit", "hof" still leaves me without a clue at first sight.

You argue against long macro names and long namespaces, but in my coding experience, people use short aliases for boost namespaces in any case, e.g. `namespace bfu = namespace boost::function_utilities;`. Even for `boost::fit` or `boost::hof`, I believe that people will make an alias, because of the repetitive `boost::` prefix. So the length of the namespace name is not an important factor IMHO. Also, we already have long namespaces in Boost, like boost::program_options, which people happily use.

I almost always create an alias to one or more boost::* namespaces. IMHO nearly everyone else too.

About the macro names, ok, you have a point, BOOST_FUNCTION_UTILITIES_* is longer than BOOST_HOF_*, but if I type it once, my editor is going to auto-complete the long name for me for the rest of the project.

There is another general argument in favour of more descriptive names, which I like: "code is written once, but read many times". Therefore, in a tradeoff between readability and writability, it is not bad to lean towards readability. If I was a random guy that reads some code that uses your library, I would scratch my head less at BOOST_FUNCTION_UTILITIES_*.

...

Also, the word 'utility' is not very descriptive, and I would prefer to move away from using it. Boost.Utility contains random things in it that are unrelated, and I don't want this library to be described as a collection of random and unrelated functions.

I tentatively agree that "Higher Order Functions" is better than "Function Utilities", but yeah it is even longer to write out. Right now we don't have a library with a three word name, this could be the first! The arguments in favor of long names also hold for `boost::higher_order_functions` and BOOST_HIGHER_ORDER_FUNCTIONS_*.

Best regards, Hans

PS: What if the library was called Boost.HigherOrderFunctions, but the namespace and macros would use boost::hof and BOOST_HOF_*? Would that be an option?

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On Mon, 2017-09-18 at 17:45 +0200, Hans Dembinski wrote:

...

On 18. Sep 2017, at 16:45, paul wrote:

But if I use the FunctionUtilities, I would find calling `boost::function_utilities::pipable` or `BOOST_FUNCTION_UTILITIES_STATIC_FUNCTION` to be just too long. 

I think HigherOrderFunctions is much more descriptive, and then I could use the namespace `hof` for short. I hope I am not bike-shedding here. I do believe that names are important, because they are the first thing you see of a library, and I believe that "obviousness" is an important factor.

I can see where you are coming from, but let me try to defend the name "Function Utilities" a bit, because although I think that using another abbreviation like "hof" is better than "fit", "hof" still leaves me without a clue at first sight.

You argue against long macro names and long namespaces, but in my coding experience, people use short aliases for boost namespaces in any case, e.g. `namespace bfu = namespace boost::function_utilities;`. Even for `boost::fit` or `boost::hof`, I believe that people will make an alias, because of the repetitive `boost::` prefix. So the length of the namespace name is not an important factor IMHO. Also, we already have long namespaces in Boost, like boost::program_options, which people happily use.

About the macro names, ok, you have a point, BOOST_FUNCTION_UTILITIES_* is longer than BOOST_HOF_*, but if I type it once, my editor is going to auto- complete the long name for me for the rest of the project.

There is another general argument in favour of more descriptive names, which I like: "code is written once, but read many times". Therefore, in a tradeoff between readability and writability, it is not bad to lean towards readability. If I was a random guy that reads some code that uses your library, I would scratch my head less at BOOST_FUNCTION_UTILITIES_*.

...

Also, the word 'utility' is not very descriptive, and I would prefer to move away from using it. Boost.Utility contains random things in it that are unrelated, and I don't want this library to be described as a collection of random and unrelated functions. I tentatively agree that "Higher Order Functions" is better than "Function Utilities", but yeah it is even longer to write out. Right now we don't have a library with a three word name, this could be the first! The arguments in favor of long names also hold for `boost::higher_order_functions` and BOOST_HIGHER_ORDER_FUNCTIONS_*.

Best regards, Hans

PS: What if the library was called Boost.HigherOrderFunctions, but the namespace and macros would use boost::hof and BOOST_HOF_*? Would that be an option?

Yea, thats what I am considering. .

On Sep 15, 2017, at 8:46 PM, Edward Diener via Boost wrote:

On 9/8/2017 7:02 AM, Matt Calabrese via Boost 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 https://github.com/pfultz2/Fit/tree/boost Docs: http://pfultz2.github.io/Fit/doc/html/doc/ http://pfultz2.github.io/Fit/doc/html/doc/

I have some questions about Fit which I can not understand by reading the docs:

1) Does Fit functionality work only with function objects as opposed to callables in general ?

In general yes, but its not the case for every adaptor.

2) Is the purpose of BOOST_FIT_STATIC_FUNCTION only to create a function object at global or namespace level ?

Yes.

3) Does BOOST_FIT_STATIC_FUNCTION only accept a function object or does it accept any callable ?

It only works with function objects, which I should document its type requirements.

4) The documentation says that "BOOST_FIT_STATIC_LAMBDA_FUNCTION can be used to the declare the lambda as a function". I am not sure what this actually means since a lambda function is a function.

Maybe that could be worded better, but it lets you declare the lambda at global or namespace scope.

5) I do not understand the purpose of the pipable adaptor. What is the point of using it ? It is mentioned as an alternative to Unified Call Syntax but, while I understand the latter, I do not understandable how pipable allows "extension methods", which is a large part of the purpose of UFCS.

That is, you want to add an extension method to a class so it can be called as `x.f(y)`. With UFCS, you can just declare a free function called as `f(x, y)` and then call it as `x.f(y)`, with the Fit library you would declare the function as pipable and then call it as `x | f(y)`.

In general I am confused by what the requirements of Fit adaptors are.

Which ones?

This may be because the terms "function objects" and "functions" are used in a way which does not follow standard C++ terminology as far as I know it.

The type requirements for the adaptors are documented as either `ConstCallable` or `ConstFunctionObject`, which follows standard C++ definitions with an additional `const` requirement.

On Sep 16, 2017, at 2:31 PM, Edward Diener via Boost wrote:

On 9/16/2017 2:14 PM, P F via Boost wrote:

...

...

5) I do not understand the purpose of the pipable adaptor. What is the point of using it ? It is mentioned as an alternative to Unified Call Syntax but, while I understand the latter, I do not understandable how pipable allows "extension methods", which is a large part of the purpose of UFCS. That is, you want to add an extension method to a class so it can be called as `x.f(y)`. With UFCS, you can just declare a free function called as `f(x, y)` and then call it as `x.f(y)`, with the Fit library you would declare the function as pipable and then call it as `x | f(y)`.

Let us suppose I have a class:

struct X { some member functions etc. };

Then I want to add a member function to class X with a prototype called void f(int) so I can call it as an extension method without modifying X. With UFCS in effect I create the free function:

void f(X px,int py) { some implementation }

and now if I have:

X x;

I can call:

x.f(3); // Works with UFCS support

successfully. Could you please show me the equivalent code that uses pipable in this same scenario ?

You could define `f` like this(using a lambda for simplification): BOOST_FIT_STATIC_LAMBDA_FUNCTION(f) = boost::fit::pipable([](X px,int py) { some implementation }); And then you can call it like this: X x; x | f(3); // Calls f as f(x, 3)

On Tue, 2017-09-19 at 23:26 -0500, Barrett Adair via Boost wrote:

On Fri, Sep 8, 2017 at 6:02 AM, Matt Calabrese via Boost < boost@lists.boost.org> wrote:

...

A formal review of the Fit library developed by Paul Fultz II starts today, September 8, and runs through September 17. (snip)

I recommend to ACCEPT the Fit library into Boost unconditionally.

Thanks you for the review.

...

  - What is your evaluation of the design?

I like it.

...

  - What is your evaluation of the implementation?

Have not reviewed in recent times. I'm very impressed with the claimed compiler support, although I'm curious to know how Clang 4 and Clang 5 fare.

I have tested on newer clang versions, but I don't have them in my travis CI to regularly test. There always seemed to be an issue using LLVM's apt repo.

I realize the importance of an implementation review for this library, so I trust that Matt will weigh this lightweight review appropriately.

...

  - What is your evaluation of the documentation?

Good. I would like to see pound-include lines added to all of the early examples. So many times as a user of Boost I have had to cross reference example code with the reference docs just to find the include path, which is annoying.

...

  - What is your evaluation of the potential usefulness of the library?

Very useful! I would have used this a handful of times at work already, had it been available in Boost.

...

  - Did you try to use the library? With which compiler(s)? Did you     have any problems?

No, at least not recently.

...

  - How much effort did you put into your evaluation? A glance? A quick     reading? In-depth study?

Tonight, about an hour reading documentation. Cumulatively, several hours over the past couple of years. I have been following Fit since before the 2016 review, so I'm thrilled to see it up for review again.

...

  - Are you knowledgeable about the problem domain?

Yes, although my experience with functional programming is mostly limited to TMP.

...

  - Were the concerns from the March 2016 review of Fit addressed?

N/A

Barrett Adair

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boo st

On 9/21/17 12:23 PM, paul via Boost wrote:

...

...

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

Hmmm - I didn't catch this. I had presumed that macros were used because there was no other way to implement the required functionality. I don't think that saving a little bit of compile time is a good justification for defining a macro. Robert Ramey