Re: [boost] [Fit] Review

Subject: Re: [boost] [Fit] Review
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-10 19:50:34

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>());

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.