Re: [boost] [Fit] Review

Subject: Re: [boost] [Fit] Review
From: Edward Diener (eldiener_at_[hidden])
Date: 2016-03-10 22:56:31

On 3/10/2016 7:50 PM, Paul Fultz II wrote:
>
>
> On Thursday, March 10, 2016 at 5:26:38 PM UTC-6, Edward Diener wrote:
>>
>> On 3/10/2016 1:46 PM, Paul Fultz II wrote:
>>>
>>>
>>> On Thursday, March 10, 2016 at 12:34:26 AM UTC-6, Vicente J. Botet
>> Escriba
>>> wrote:
>>>>
>>>> Le 10/03/2016 01:12, Paul Fultz II a écrit :
>>>>>
>>>>> On Wednesday, March 9, 2016 at 8:50:01 AM UTC-6, Zach Laine wrote:
>>>>>>
>>>>>> There are a number of adaptors that I find have obscure names:
>>>>>>
>>>>>> by() is a projection; why not call it project()?
>>>>>>
>>>>> I chose `by` because of how it is used linguistically. That is you
>>>> write:
>>>>> `sort(v.begin(), v.end(), by(&Employee::Name, _<_))` which I read "sort
>>>> by
>>>>> employee name". Also, if I write `by(decay, construct<std::tuple>())`,
>> I
>>>>> read
>>>>> "construct tuple by decay". In haskell, it uses `on` for this adaptor,
>>>> which
>>>>> is what I originally called it. `project` is a more straighforward
>> name.
>>>> `by` is a comparator while `projet` is a projection.
>>>>
>>>
>>> By is a projection in the example of `construct`.
>>>
>>>
>>>>>
>>>>>
>>>>>> compress() is defined in terms of "fold", with a link to the Wikipedia
>>>>>> page
>>>>>> for same; why not call it foldl()?  Similarly, reverse_compress()
>>>> should
>>>>>> be
>>>>>> foldr().
>>>>>>
>>>>> Not `foldr`. This is because `foldr` is symetrical. For example,
>> `foldl`
>>>> and
>>>>> `foldr` should produce the same results:
>>>>>
>>>>>        foldl(_+_, std::string())("Hello", "-", "world"); //
>> "Hello-world"
>>>>>        foldr(_+_, std::string())("Hello", "-", "world"); //
>> "Hello-world"
>>>> It would be good to use a binary function that has two different types
>>>> to see the difference in behavior.
>>>>
>>>
>>> Yes, you could do `flip(_+_)` to get the same effect. However, I want the
>>> callback to be consistent with compress reverse_compress, so it always is
>>> called with f(state, element). Also, `fold` and `reverse_fold` are
>> commonly
>>> used names in C++, so I would like to keep it more consistent with those
>>> usage. If the user wants `fold_right`, it can be easily written, like
>> this:
>>> `auto fold_right = compose(compress, flip)`.
>>>
>>>
>>>>> However, compress and reverse_compress work like this:
>>>>>
>>>>>        compress(_+_, std::string())("Hello", "-", "world"); //
>>>> "Hello-world"
>>>>>        reverse_compress(_+_, std::string())("Hello", "-", "world"); //
>>>>> "world-Hello"
>>>>>
>>>>> I was reluctant to call it `fold` as it seems to imply some data
>>>> structure
>>>>> to
>>>>> fold over, whereas this is simply an adaptor. I used the word compress
>>>> as
>>>>> its
>>>>> a another name used for fold. However, it seems more people would
>> prefer
>>>> to
>>>>> use `fold` and `reverse_fold`.
>>>> You are folding the parameter pack. I like those. and I would like also
>>>> fold_left, fold_right.
>>>>>
>>>>>
>>>>>> flow() is a super obscure name!  Why is this not reverse_compose() or
>>>>>> similar?
>>>>>>
>>>>> I actually got the name from underscore.js. The reason why I chose this
>>>> name
>>>>> instead of `reverse_compose` is because when this is used with pipable
>>>>> functions it seems confusing:
>>>>>
>>>>>        reverse_compose(
>>>>>            filter([](int i) { return i < 3; }),
>>>>>            transfrom([](int i) { return i*i; })
>>>>>        )(numbers);
>>>>>
>>>>> With the word 'reverse' there, it almost looks as if it computes the
>>>>> pipeline
>>>>> in reverse order, which it doesn't. So I would prefer a name without
>>>> reverse
>>>>> in it.
>>>> Would pipe works better?
>>>>
>>>
>>> pipe could work, I don't know how others feel about such a name.
>>>
>>>
>>>>>
>>>>>
>>>>>> indirect() could more easily be understood if it were called deref()
>> or
>>>>>> dereference().
>>>>>>
>>>>> Well, `indirect` is how it is commonly called in libraries such as
>>>>> Boost.Range, range-v3, and PStade libraries. So I would like to keep
>> the
>>>>> name
>>>>> consistent with other similar constructs.
>>>>>
>>>> Could you tell us what indirect is in range-v3, what is the signature
>>>> and the semantics. I don't see it is used for the same purposes, but
>>>> maybe I'm wrong.
>>>>
>>>
>>> It dereferences the values in a range. They both are functors that will
>>> dereference the parametrized type.
>>>
>>>
>>>>
>>>> ref(flv) is a callable that wraps an lv
>>>> indirect(ptr) is a callable that wraps a ptr and deref it before calling
>>>>
>>>
>>> Well, it can be used with anything that is dereferenceable. So it works
>> with
>>> boost::optional as well.
>>>
>>>
>>>>
>>>> deref/dereference will only relate the dereference action, but not the
>>>> call.
>>>>
>>>> deref_on_call is too long?
>>>>
>>>
>>> Yes it is.
>>>
>>>
>>>>>> I don't feel as strongly about partial(), but I think it might be
>>>> clearer
>>>>>> if
>>>>>> it were called partial_apply() or curry().
>>>>>>
>>>>> Hmm, I don't think a name with `apply` in it is good name for an
>>>> adaptor.
>>>> Maybe you could explain why curry is not a good name here? What is the
>>>> difference between partial an currying a function.
>>>>>
>>>>>
>>>>>> conditional() should be called invoke_first(), call_first_of(), or
>>>>>> similar.
>>>>>>     I
>>>>>> find it too easy to confuse with if_().
>>>>>>
>>>> I agree conditional is not a good name.
>>>>
>>>>
>>>>> I see, I was trying to describe an adaptor where you could put
>> functions
>>>>> with
>>>>> conditions in it. Other people, seem to prefer a name like `linear`, so
>>>> I
>>>>> might use that instead.
>>>> This function is related to match. The difference is that one select the
>>>> best matching overload and those must be exclusive and the other the
>>>> first matching overload and the match can be inclusive. I would like to
>>>> see the semantics on the name, but I have not a concrete proposal.
>>>>
>>>> We need to take in account that this is an adaptor, so there is no call,
>>>> so `invoke_first` or call_first_off will not work. Those functions
>>>> creates another function object that applies a different algorithm to
>>>> select the overloaded functions when called.
>>>>
>>>> I've used
>>>>        overload -> match
>>>>        first_overload -> conditional
>>>>
>>>
>>> I would prefer `match` and `linear`.
>>>
>>>
>>>>
>>>> I prefer those, but I'm not yet happy with. Naming is difficult.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>      * Documentation
>>>>>>
>>>>>> The Quick Start section is good at showing a compelling motivating
>> case
>>>>>> for
>>>>>> using the library.  The result at the end of the section looks like a
>>>> very
>>>>>> good start on a "Scrap Your Boilerplate" solution for simple,
>>>> dump-style
>>>>>> printing.  It's relatively easy to read and write.
>>>>>>
>>>>>> The documentation is what needs the most work, from what I've seen.
>>>>    The
>>>>>> Quick
>>>>>> Start is quite nice, but then there are some things that are
>>>>>> underexplained
>>>>>> in
>>>>>> the subsequent Overview and Basic Concepts sections.  For instance,
>> why
>>>>>> are
>>>>>> these macros used everywhere and what do they do?  They should be used
>>>> for
>>>>>> the
>>>>>> first time after showing what the expanded code looks like.
>>>>>
>>>>> I don't think the docs should show the expansion of the macros, that is
>>>> part
>>>>> of the implementation and not interface. I could show an "idea" what is
>>>> is
>>>>> expanding to, with explanation of what else it is doing beyond the
>>>> simple
>>>>> explanation.
>>>> I agree hat it is absolutely needed to show the exact expansion, but it
>>>> is clear that the user wants to know what is behind the scenes.
>>>
>>>
>>> I strongly disagree. Documentation is about documenting the interface,
>> not
>>> the
>>> implementation. Mainly because the implementation could change or vary
>>> between
>>> platforms while the interface would remain the same. If the user wants to
>>> know
>>> about the implementation, they can look at the source code or in the
>> case of
>>> macros, look at the preprocessed output.
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> This will
>>>>>> justify
>>>>>> their use for those users that want them, and show those that don't
>>>> what
>>>>>> they
>>>>>> can write instead.
>>>>>>
>>>>>
>>>>> I could show an alternative without the macros, but I would prefer to
>>>> put
>>>>> that
>>>>> in the Advance section. In the introduction of the library, I would
>>>> rather
>>>>> show the constructs that can be easily used without a need to explain a
>>>>> bunch
>>>>> of caveats.
>>>> I will put all of them in the advanced section. I would no mention them
>>>> in the introduction as the user can create the HOF locally or use a
>>>> factory.
>>>>
>>>
>>> Being able to declare the functions as global variables is a key feature
>> of
>>> the library. The example in the quick start guide shows how to implement
>>> many
>>> things without the need for a lot of template boilerplate. For those
>> users
>>> that prefer not to use global function objects(and I have yet seen a
>>> compelling reason not to use them) can look at the 'Advanced' section.
>>>
>>> I realize now, that I need to spend more discussing the advantages of
>> using
>>> global function objects(more composability), and address some of the
>>> misperceived issues with these global function objects(they can be in a
>>> namespace to avoid name conflicts and are not mutable so it won't be a
>>> problem
>>> in mutilthreaded environments).
>>
>> You do need to explain why "global variables" is a key feature of the
>> library. I cannot understand from your docs why there is any
>> disadvantage instantiating Callables locally as opposed to globally
>> other than the usual fact that a local object can go out of scope.
>>
>
> We can define functions by simply composing other functions together and we
> don't need to write awkward syntax nor template boilerplate for it. For
> example, we can simply write `make_tuple` like this:
>
>      BOOST_FIT_STATIC_FUNCTION(make_tuple) = by(decay,
> construct<std::tuple>());

What is the type of by(decay,construct<std::tuple>()) ?

I thought I would be able to write locally:

auto make_tuple(by(decay,construct<std::tuple>()));

but evidently you are saying that does not work.

>
> Alternatively, you could write a factory function for this instead:
>
>      constexpr auto make_tuple()
>      {
>          return by(decay, construct<std::tuple>());
>      }
>
> Of course this requires the user to write `make_tuple()(xs...)`, which I
> don't
> think any user would expect this, especially since `std::make_tuple` doesn't
> work like that. So instead, it can be wrapped in a template function:
>
>      template<class... Ts>
>      constexpr auto make_tuple(Ts&&... xs)
>          -> declype(by(decay,
> construct<std::tuple>())(std::forward<Ts>(xs)...))
>      {
>          return by(decay, construct<std::tuple>())(std::forward<Ts>(xs)...);
>      }
>
> Which is a lot of boilerplate to compose some simple functions.
>
>
>> If
>> there really is some other reason it is completely lost to me. If you
>> would like to point to me in your doc where you explain the use of
>> "global variables" as being a key feature of your library or as being
>> necessary to use the functionality of your library, I would be glad to
>> read about it and ask further about it here.
>>
>
> It is not a necessary feature, but it is an important feature nonetheless,
> and
> I need to spend more time explaining its usefulness.
>
>
>> If, OTOH, it is just your
>> preference to use global objects as opposed to the various forms of
>> local instantiation, I really wish you would just say that rather than
>> acting like your library does not work correctly somehow other than with
>> global variables.
>>
>
> The composability of the adaptors and functions applies to both global and
> local functions. The adaptors provide a simple way to create functions to be
> passed locally, such as creating a comparator for std::sort(ie
> `std::sort(first, last, by(&employee::name, _<_))`). However, they can also
> be
> used to define global/free functions in a much simpler way as well(such as
> writing `std::make_tuple`), which I think is equally important.
>
> Furthermore, in a quick start guide or introduction, I want to be able to
> demonstrate the capabilities of the library in a way to show its usefulness,
> and why someone would choose to use this library. Perhaps, you are ok with
> awkward syntax or template boilerplate in writing your functions, and would
> prefer to only use the adaptors for local functions. However, there are
> others
> who will find writing global function objects with the adaptors very useful,
> so I want to be able to show those capabilities in the introduction of the
> library. I do, however, need to discuss some of the misperceived issues with
> using global function objects in the documentation. As the issues raised in
> the review were:
>
> 1) Namespacing - This is not a problem because all global function objects
> can
> be placed in a namespace.
>
> 2) Global state - This is not a problem either, because
> `BOOST_FIT_STATIC_FUNCTION` declares the function object const with
> constexpr.
> So there can be no mutable or changing state internally in the object,
> and it must be constructed without side effects.
>
> I think addressing these concerns in the documentation will help put users
> at
> ease when using global function objects, so they have no problem taking
> advantage of the adaptors to build function objects globally.
>
>
>> Maybe I have missed something but I have the intuition
>> that many others have missed it likewise, from the responses of others
>> about this issue.
>>
>
> I agree with your intuition, and I believe the documentation should spend
> more
> time discussing these issues. I hope my email has made it somewhat clearer
> to
> you.