"Vicente J. Botet Escriba" wrote:

We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance - Your name

Rob Stewart

- Your knowledge of the problem domain.

I'm neither expert nor overly experienced due to the need to support LCD compilers, but reasonably well educated on the topic.

You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness

Without meaning to beat a dead horse, I must say that the initial documentation page is off-putting. The TOC is very long, and there's very little to help me understand the point. For example, the Introduction tells me that the library, "provides utilities for functions and function objects." My first question is, why do I need utilities for such things? I've been writing them for years without difficulty. The page descibes language facilities it uses, but I don't know why nor do I care without knowing how the library will actually benefit me. The first page of the documentation has to sell me on the library. It should also clearly identify the audience. If a potential user is inexperienced with TMP, will the library hurt or help? If the user is a heavy user of lambdas how will the library help? If the user can't use C++11, will the library help? If you make me work this hard to discover whether I can get value from your library, I'm going to ignore it. Once you've identified your audience, ensure that the terminology and examples are targeted to that audience. If you think your library should appeal only to expert functional or TMP programmers, say so and then speak to them in language they will recognize. If your library should appeal to folks that don't understand much about functional programming, but will lead them along that path, be sure to describe things in terms non-functional programmers will understand. If your library should appeal to a broad range of users, then you may well need more than one kind of introduction or tutorial. That is, you might give those with more experience or background a quick walk through the range of features and lead them quickly to more advanced examples and the reference material, while you give the rest a gentler introduction. Without that information, I really don't know how useful the library is to me and the design, implementation, and tests are largely irrelevant. That said, I will continue to give more impressions based upon reading through more of the documentation. Quick Start This page is broken into sections with headings and provides a TOC, yet the sections are not independent. Instead, the content must be read from top to bottom to make sense. Therefore, the TOC is misleading. Function Objects You state that the function call operator is always declared const. On what basis do you make that claim? It may be a reasonable, even common thing to do, but you should justify your assertion. That such is needed for working with Fit is not even relevant at this point in the documentation. You were just describing a function object. Mentioning mutable_ at this point is most definitely too much information. You write, "construct the class as a global variable," which is phrased incorrectly. That should be something like, "construct an instance of the class in a global variable." What's more, you should rephrase that sentence to highlight Fit's role better: "Fit provides an easy means to put an instance of the class in a global variable, with no ODR problems, that can be invoked like an ordinary function." That would explain the point a little better and specifically address the ODR concerns raised during the review. Adaptors s/Now we have/Now that we have/ You note that the function could be made pipable without explaining what that means or why it's valuable. This sort of thing works better if you provide a use case and tentative syntax and then show how Fit can make that a reality. You go on to discuss the variants without the "_adaptor" suffix without any explanation of why that is of any use. You assume that the reader can infer, from the terse examples offered, their own uses. At the very least, some description of, or reference to, partial application of functions would be of help here. Still, the point is that you presume much more than I think you should of the reader. Lambdas This section looks ugly, because of all of the macros you've used, and makes no attempt whatsoever to explain why any of the things shown would be useful. Overloading This section assumes that the reader will make inferences that initializing lambdas at compile time and doing something special with overloading that we apparently don't get from the language itself, Fit improves the programmer's life. Guess what? That assumption is faulty. I can see that conditional might be preferable to match because it doesn't apply overload resolution and, therefore, doesn't lead to the same ambiguities. However, the question remains why I would want either in the first place. The last sentence, refers to the example that follows it, but is worded as if it refers to the previous example as the sentence before it did. To make things worse, the helper function mentioned is not named, though it can be inferred from reading the following code, and it is not explained. Tuples There's an awful lot packed into the first three sentences and nothing in them actually explains what the following code does. I really don't know what, "Since we can't use a lambda inside of decltype we just put identity instead," is supposed to mean. I can see that you reference identity in the example code, but I really don't know why. (I can reason that somehow identity will be the tuple type, but that's a lot to swallow with no explanation.) Recursive I can guess that the point of this section is to make the print function object print objects within objects using recursive invocations of print, but you need to describe that and even show sample output for some inputs. (That illustration of outputs for inputs idea should be applied to each section.) Variadic This section makes the most sense of them all. You actually explained why making the function variadic would be useful and the modification was explained and then shown. After having worked to get this far through the documentation, I would have walked away were I not writing this review. You've thrown mutable_, pipable_adaptor, match, conditional, by, unpack, identity, fix, and adl::adl_begin() at me with very poor explanations to develop an example that I can only guess might have some narrow application to my code. Frankly, I would have walked away before I reached the end. More examples "a key point of many of these utilities is that they can solve these problems with much simpler constructs than whats traditionally been done with metaprogramming" That's it! Why wasn't that on the first page! That's the thesis of this library. That's what you need to focus on as you write the documentation. Show how Fit makes things simpler. Highlight my pain and show me how to reduce or eliminate it. What's more, this page is exactly what I expected to be the Motivation page. You've described real world problems and shown how Fit can solve or simplify them. In the Projections section, you wrote, "Instead of writing the projection multiple times in algorithms." I can infer that you mean one had to write ".year_of_birth" multiple times, but I'm not familiar with the term "projection" in that context. Be careful what you assume your readers know. Near the bottom, you refer to "UFCS" without defining the acronym or linking to a definition. s/template classes/class templates/ s/bit or/bitwise OR/ Overview Why is the third page in the documentation called "Overview"? That's normally what one expects right at the beginning. In reality, I think you mean to call this page something like, "Library Features." There really should be an introductory sentence explaining the point of this page, which I infer to be a summary of the key features of the library. Static Function Adaptor I have no idea what, "A static function adaptor is a function adaptor that doesn't have a functional form," is supposed to mean. Decorator I have no idea what, "A decorator is a function that returns a function adaptor," is supposed to mean. Semantics This section is in the middle of sections describing main areas of functionality in the library, so it's out of place. This is especially true given the links in the page TOC. This content should follow the TOC with no heading as part of a general introduction to the page. Signatures This section also seems to be a general statement about content to be found elsewhere in the documentation. It isn't a feature of the library, so it confuses me based upon what I was guessing this page was about. That's as far as I got in the documentation. I'm really confused as to the real purpose and scope of this library and on that basis I don't think it should be accepted in its present form. Once the purpose and scope are well defined, it will be possible to judge how well it fits and whether that should be part of Boost.

- Did you attempt to use the library?

No

- How much effort did you put into your evaluation of the review?

Several hours, including following the review discussions. ___ Rob

On Sunday, March 20, 2016 at 9:07:53 PM UTC-5, Gavin Lambert wrote:

On 21/03/2016 14:20, rstewart wrote:

...

You state that the function call operator is always declared const. On what basis do you make that claim? It may be a reasonable, even common thing to do, but you should justify your assertion.

On a related note, at least in the codebases I tend to work with, const function objects are the exception rather than the rule.

However, with lambdas it is the opposite. Lambdas default to const and then an explicit mutable keyword is needed for a mutable function object.

While it's likely that some of these could be made const with a little restructuring,

Just like lambdas, mutability is not prohibited in Fit, just explicit.

I think it's still true that mutable functions are useful in more cases -- despite being vulnerable to surprise copies and thread-safety issues.

(Having said that, this may be because boost::bind is used in most cases where const function objects would otherwise be used, so that custom function objects are typically only created where they need to be mutable; the code does not yet make extensive use of lambdas. But I don't think my experience is unique.)

I would like to note, that the const requirement only applies to function objects. You can pass member function pointers to member functions that are mutable.

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

On Monday, March 21, 2016 at 4:53:58 PM UTC-5, Edward Diener wrote:

On 3/21/2016 2:04 PM, Paul Fultz II wrote:

...

On Sunday, March 20, 2016 at 9:07:53 PM UTC-5, Gavin Lambert wrote:

... snipped

...

...

I think it's still true that mutable functions are useful in more cases -- despite being vulnerable to surprise copies and thread-safety issues.

(Having said that, this may be because boost::bind is used in most cases where const function objects would otherwise be used, so that custom function objects are typically only created where they need to be mutable; the code does not yet make extensive use of lambdas. But I don't think my experience is unique.)

I would like to note, that the const requirement only applies to function objects. You can pass member function pointers to member functions that are mutable.

You probably meant to say "You can pass member function pointers for mutable member functions". I would also strongly suggest you use the terms 'const' and 'non-const' when referring to member functions rather than 'const' and 'mutable' in your documentation. The reason I believe this is less confusing is because 'mutable' is a C++ keyword and as a keyword means something entirely different from how you are using it in your documentation.

How is it different? The mutable keyword is used to signify the function as non-const. That is when I write: auto f = [] mutable {}; It is the equivalent of writing a class with a "non-const" call operator like this: struct local_f{ void operator()() {} // No const here }; auto f = local_f{};

So I think what you really meant to say above would be better expressed as "You can pass member function pointers for non-const member functions".

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

On 03/21/2016 07:04 PM, Paul Fultz II wrote:

However, with lambdas it is the opposite. Lambdas default to const and then an explicit mutable keyword is needed for a mutable function object.

The const-by-default lambdas are the black swans of C++. The fit documentation should at least provide a design rationale for its const-by-default decision which is addressed at those who are unfamiliar with the virtues of immutability. Another concern with fit::mutable_ is its name. As you have already gone down the type-traits naming path with fit::decay, it may be an idea to rename fit::mutable_ to fit::remove_const.

On Tuesday, March 22, 2016 at 3:52:23 AM UTC-5, Bjorn Reese wrote:

On 03/21/2016 07:04 PM, Paul Fultz II wrote:

...

However, with lambdas it is the opposite. Lambdas default to const and then an explicit mutable keyword is needed for a mutable function object.

The const-by-default lambdas are the black swans of C++.

Other than working around bugs in MSVC, I have never used a mutable lambda in any of the codebases I have worked on. I find it quite surprising that const lambdas are considered a "black swan".

The fit documentation should at least provide a design rationale for its const-by-default decision which is addressed at those who are unfamiliar with the virtues of immutability.

Another concern with fit::mutable_ is its name. As you have already gone down the type-traits naming path with fit::decay, it may be an idea to rename fit::mutable_ to fit::remove_const.

No, fit::decay works something like this(ignoring unwrapping references): template<class T> std::decay_t<T> decay(T&& x) { return std::forward<T>(x); } It simply transforms the type according to the rules of the type trait std::decay. So, fit::remove_const would imply the same: template<class T> remove_const_t<T> decay(T&& x) { return const_cast(x); } However, fit::mutable_ does not work that way at all. It is an adaptor, so it only accepts a function object, and returns a function object. Even more so,it does not remove const at all, not even from the function object call operator. Rather, fit::mutable_ creates a new function object with const added to the call operator. However, calling it fit::add_const would imply that it would prevent mutablilty which is not the case. So I think fit::mutable_ is a much better name, as it actually describes what is happening to the object using the same terminology that is used for lambdas. Plus, it the describes the C++ mechanism that is used to implement fit::mutable_ as well(that is the adaptor uses the mutable keyword).

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

On Sunday, March 20, 2016 at 8:21:09 PM UTC-5, rstewart wrote:

"Vicente J. Botet Escriba" javascript:> wrote:

...

We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance - Your name

Rob Stewart

...

- Your knowledge of the problem domain.

I'm neither expert nor overly experienced due to the need to support LCD compilers, but reasonably well educated on the topic.

...

You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness

Without meaning to beat a dead horse, I must say that the initial documentation page is off-putting. The TOC is very long, and there's very little to help me understand the point. For example, the Introduction tells me that the library, "provides utilities for functions and function objects." My first question is, why do I need utilities for such things? I've been writing them for years without difficulty. The page descibes language facilities it uses, but I don't know why nor do I care without knowing how the library will actually benefit me. The first page of the documentation has to sell me on the library. It should also clearly identify the audience. If a potential user is inexperienced with TMP, will the library hurt or help? If the user is a heavy user of lambdas how will the library help? If the user can't use C++11, will the library help? If you make me work this hard to discover whether I can get value from your library, I'm going to ignore it.

The library does require C++11 as mentioned in the first sentence. However, this is somewhat ambiguous, so its best too look at what compiler are supported. The Fit library provides simple function utilities that can provide a simpler alternative to many template metaprogramming constructs. It doesn't require expertise in TMP, and the point of the "relevant" section is it doesn't require expertise in functional programming as well. Although background in both of these areas can be helpful.

Once you've identified your audience, ensure that the terminology and examples are targeted to that audience. If you think your library should appeal only to expert functional or TMP programmers, say so and then speak to them in language they will recognize. If your library should appeal to folks that don't understand much about functional programming, but will lead them along that path, be sure to describe things in terms non-functional programmers will understand. If your library should appeal to a broad range of users, then you may well need more than one kind of introduction or tutorial. That is, you might give those with more experience or background a quick walk through the range of features and lead them quickly to more advanced examples and the reference material, while you give the rest a gentler introduction.

Without that information, I really don't know how useful the library is to me and the design, implementation, and tests are largely irrelevant. That said, I will continue to give more impressions based upon reading through more of the documentation.

Quick Start

This page is broken into sections with headings and provides a TOC, yet the sections are not independent. Instead, the content must be read from top to bottom to make sense. Therefore, the TOC is misleading.

A TOC is provided for all headers on all pages.

Function Objects

You state that the function call operator is always declared const. On what basis do you make that claim?

This I plan to discuss more. There are two main reason for this: 1) Mutable function objects are problematic with many surprises. Most of the time `std::ref` should be used instead. 2) With C++ lambdas, mutability is explicit, and the Fit library follows this convention as well.

It may be a reasonable, even common thing to do, but you should justify your assertion. That such is needed for working with Fit is not even relevant at this point in the documentation. You were just describing a function object. Mentioning mutable_ at this point is most definitely too much information.

I just wanted to make clear that mutability is not completely prohibited.

You write, "construct the class as a global variable," which is phrased incorrectly. That should be something like, "construct an instance of the class in a global variable." What's more, you should rephrase that sentence to highlight Fit's role better: "Fit provides an easy means to put an instance of the class in a global variable, with no ODR problems, that can be invoked like an ordinary function." That would explain the point a little better and specifically address the ODR concerns raised during the review.

Yes, I think that is better phrased. Although, I plan to add a deeper discussion about how functions are declared.

Adaptors

s/Now we have/Now that we have/

You note that the function could be made pipable without explaining what that means or why it's valuable. This sort of thing works better if you provide a use case and tentative syntax and then show how Fit can make that a reality.

You go on to discuss the variants without the "_adaptor" suffix without any explanation of why that is of any use.

The point is to show both forms are available, and the later examples use the functional forms. Perhaps it would be better to not discuss the "_adaptor" form in this section at all.

You assume that the reader can infer, from the terse examples offered, their own uses. At the very least, some description of, or reference to, partial application of functions would be of help here. Still, the point is that you presume much more than I think you should of the reader.

Lambdas

This section looks ugly, because of all of the macros you've used, and makes no attempt whatsoever to explain why any of the things shown would be useful.

I write, "Instead of writing function objects which can be a little verbose, we can write the functions as lambdas instead.". That is why they are useful. How should I word it to make it more clearer?

Overloading

This section assumes that the reader will make inferences that initializing lambdas at compile time and doing something special with overloading that we apparently don't get from the language itself, Fit improves the programmer's life. Guess what? That assumption is faulty.

The example I give for match doesn't have much advantage. It is only used to transition to 'conditional'. However, match is useful for when you are using visitors.

I can see that conditional might be preferable to match because it doesn't apply overload resolution and, therefore, doesn't lead to the same ambiguities. However, the question remains why I would want either in the first place.

The last sentence, refers to the example that follows it, but is worded as if it refers to the previous example as the sentence before it did. To make things worse, the helper function mentioned is not named, though it can be inferred from reading the following code, and it is not explained.

I'll move through this more slowly.

Tuples

There's an awful lot packed into the first three sentences and nothing in them actually explains what the following code does.

Hmm. The first three sentences explain what the 'for_each_tuple' code does. I don't understand the disconnect here. Although, I could add an example of using 'for_each_tuple', which might help.

I really don't know what, "Since we can't use a lambda inside of decltype we just put identity instead," is supposed to mean. I can see that you reference identity in the example code, but I really don't know why. (I can reason that somehow identity will be the tuple type, but that's a lot to swallow with no explanation.)

Your reasoning about it is wrong, and the code is wrong, as pointed out by this issue here: https://github.com/pfultz2/Fit/issues/131 Either way, I plan to drop this part, as it just complicates the quick start guide.

Recursive

I can guess that the point of this section is to make the print function object print objects within objects using recursive invocations of print, but you need to describe that and even show sample output for some inputs. (That illustration of outputs for inputs idea should be applied to each section.)

Some examples would be good.

Variadic

This section makes the most sense of them all. You actually explained why making the function variadic would be useful and the modification was explained and then shown.

After having worked to get this far through the documentation, I would have walked away were I not writing this review. You've thrown mutable_, pipable_adaptor, match, conditional, by, unpack, identity, fix, and adl::adl_begin() at me with very poor explanations to develop an example that I can only guess might have some narrow application to my code. Frankly, I would have walked away before I reached the end.

More examples

"a key point of many of these utilities is that they can solve these problems with much simpler constructs than whats traditionally been done with metaprogramming"

That's it! Why wasn't that on the first page! That's the thesis of this library. That's what you need to focus on as you write the documentation. Show how Fit makes things simpler. Highlight my pain and show me how to reduce or eliminate it.

What's more, this page is exactly what I expected to be the Motivation page. You've described real world problems and shown how Fit can solve or simplify them.

In the Projections section, you wrote, "Instead of writing the projection multiple times in algorithms." I can infer that you mean one had to write ".year_of_birth" multiple times, but I'm not familiar with the term "projection" in that context. Be careful what you assume your readers know.

Near the bottom, you refer to "UFCS" without defining the acronym or linking to a definition.

s/template classes/class templates/ s/bit or/bitwise OR/

Overview

Why is the third page in the documentation called "Overview"? That's normally what one expects right at the beginning. In reality, I think you mean to call this page something like, "Library Features."

I was planning to rename this page to "Definitions".

There really should be an introductory sentence explaining the point of this page, which I infer to be a summary of the key features of the library.

Static Function Adaptor

I have no idea what, "A static function adaptor is a function adaptor that doesn't have a functional form," is supposed to mean.

Why? What part is not clear?

Decorator

I have no idea what, "A decorator is a function that returns a function adaptor," is supposed to mean.

Why? What part is not clear?

Semantics

This section is in the middle of sections describing main areas of functionality in the library, so it's out of place. This is especially true given the links in the page TOC. This content should follow the TOC with no heading as part of a general introduction to the page.

This is not describing functionality, but just notation used in the library.

Signatures

This section also seems to be a general statement about content to be found elsewhere in the documentation. It isn't a feature of the library, so it confuses me based upon what I was guessing this page was about.

It is a feature. Since functions are designed this way, it makes it easier to pass them to other functions(such as writing `compose(unpack, by)`).

That's as far as I got in the documentation. I'm really confused as to the real purpose and scope of this library and on that basis I don't think it should be accepted in its present form. Once the purpose and scope are well defined, it will be possible to judge how well it fits and whether that should be part of Boost.

There has been a lot of feedback about the documentation and I plan to rewrite a lot of the documentation.

...

- Did you attempt to use the library?

No

...

- How much effort did you put into your evaluation of the review?

Several hours, including following the review discussions.

Thanks Rob for the review.

___ Rob

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

On Tuesday, March 22, 2016 7:01 PM, Gavin Lambert wrote:

On 22/03/2016 06:55, Paul Fultz II wrote:

...

...

Function Objects

You state that the function call operator is always declared const. On what basis do you make that claim?

This I plan to discuss more. There are two main reason for this:

1) Mutable function objects are problematic with many surprises. Most of the time `std::ref` should be used instead.

These are not exclusive. I've often created a "real" function object (custom struct/class) with non-const call operator, and then used bind(ref(func), params...) on it. This works because bind can call both

const and non-const functors and can auto-unwrap ref().

This doesn't really matter. `reference_wrapper` has a call operator that is only const. Having a non-const overload for `reference_wrapper` would imply that the function could change what `reference_wrapper` points to, but it can't. So there is no reason to have a non-const overload. Same applies to using `fit::indirect(&f)`.

The output of >bind() might be a const functor, but its input is not required to be.

(And in some cases using ref() is not possible, as the functor must be returned out of the original scope. Admittedly this is harder to do correctly with non-const functors than with const ones, but shared_ptr and friends can come to the rescue here, albeit with some minor

performance cost.)

With Fit, you can do `fit::indirect(std::make_shared())` to turn a shared_ptr that points to a Callable into a FunctionObject. Additionally, if you absolutely know what you are doing, you can still use `fit::mutable_` as well.

As I mentioned in the other branch of this thread, my experience has been that any time I want a const functor I use bind() or a lambda, and any time I want a non-const functor I write a custom struct. Thus, the only times I actually implement operator() myself, it is never const. I don't know how wide-spread this practice is, but I don't believe it is unique.

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

...

On 23/03/2016 13:33, paul Fultz wrote: This doesn't really matter. `reference_wrapper` has a call operator

On Tuesday, March 22, 2016 8:10 PM, Gavin Lambert wrote: that is

...

only const. Having a non-const overload for `reference_wrapper` would imply that the function could change what `reference_wrapper` points to, but it can't. So there is no reason to have a non-const overload.

Well, I'm mostly using boost::reference_wrapper at the moment, which lacks a call operator, so boost::bind does have to explicitly unwrap it (though it does do this internally, so user code doesn't care). But that's good to know; I hadn't noticed that the C++11 standard had introduced this.

...

Same applies to using `fit::indirect(&f)`.

So, you're saying that this is valid code (noting lack of const)?

Yes that code is valid.

struct F { F() : value(0) {} void operator()(int a) { value += a; } int value; } f;

fit::indirect(&f)(15); fit::indirect(&f)(2); assert(f.value == 17);

If so, then that satisfies my concern.

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