Subject: Re: [boost] [Fit]Â formal review starts today
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2016-03-04 15:27:08
On Friday, March 4, 2016 at 12:56:05 PM UTC-6, Vicente J. Botet Escriba
wrote:
>
> Le 04/03/2016 03:16, paul Fultz a écrit :
> >> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watan..._at_[hidden]
> <javascript:>> wrote:
> >>> AMDG
> Thanks Steven for all these useful remarks.
> >>> - How much effort did you put into your evaluation of the review?
> >>>
> >> ~5 hours. I only finished half a dozen
> >> files, but I've seen enough problems that
> >> I feel justified in stopping now.
> >>
> >> Critical points:
> >>
> >> - Different functions in the library do not
> >> have a consistent interface for similar
> >> variations:
> >> - always/always_ref vs. capture/capture_forward/capture_decay vs
> >> partial w/ std::ref.
> >>
> >> - The library reinvents (poorly) its own versions
> >> of several components:
> >> compressed_pair <-> boost::compressed_pair,
> > As I recall boost::compressed_pair is never empty. In addition, it
> doesn't
> > support constexpr. Plus, I structured it in a way to avoid bugs with gcc
> when
> > doing EBO with constexpr.
> Could you provide a patch for boost::compressed_pair that is backward
> compatible and provides whatever you need?
>
Nope. Compressed pair is designed such that &x.first() != &x.second() is
always the case. As some users may make assumptions about that.
> >> placeholders <-> lambda, bind, phoenix
> > Doesn't work with constexpr, and/or is not SFINAE-friendly nor does it
> handle
> > perfect forwarding for more than 2 arguments.
> The same here.
>
Making bind SFINAE-friendly breaks backwards compatibility. However, the
lazy adapter passes the same tests for Boost.Bind, plus with constexpr.
>
> >> - Some library functions require function objects
> >> to have a const operator(), without any rhyme
> >> or reason. e.g. apply requires Callable, but
> >> apply_eval requires ConstCallable.
> > This is partly because of the fold done for older compiler. I could
> easily
> > lift this restriction. Almost all of the adaptors use `ConstCallable`.
> IIUC Steven, he is requesting to use Callable everywhere. Steven could
> you confirm?
>
The library could switch to using `Callable`, if everyone feels that is
better. However, I honestly prefer to make mutable explicit. I would like
to hear feedback from other reviewers as well.
>
> >> 107: I have no idea why you made alias_value variadic.
> >> The extra arguments are unused, their presence is
> >> undocumented, and all calls from inside the library
> >> (pack.hpp, combine.hpp, and detail/compressed_pair.hpp)
> >> that use them, could just as well be written without.
> > This is to make `alias_value` depend on a template parameter because
> compiler
> > eagerly instantiate template parameters.
> Please could a clear explanation be added in the documentation?
>
I don't know if it should be added to the documentation as its a
implementation detail. Perhaps an extra comment could help.
> >
> >> arg.hpp:
> >>
> >> 29: /// template<class IntegralConstant>
> >> /// constexpr auto arg(IntegralConstant);
> >> ///
> >> /// template<std::size_t N, class... Ts>
> >> /// constexpr auto arg_c(Ts&&...);
> >> arg and argc should do the same thing. It looks
> >> like arg creates a function object, while arg_c
> >> evaluates it directly.
> > Yes, this because you write `arg(int_<5>)(...)` and `arg_c<5>(...)`. In
> the
> > future, I may make this a template variable(so `arg_c<5>` will be a
> function
> > object).
> What prevents you to doing it now?
>
Time constraints mainly, but this applies to both arg_c and if_c.
> >
> >> 86: make_args_at(...)(..., make_perfect_ref(...)...)
> >> ADL...
> >>
> >> 95: get_args<N>(...)
> >> ADL
> Associated those to the ADL issue.
> >> by.hpp:
> >>
> >> 8: BOOST_FIT_GUARD_FUNCTION_ON_H
> >> The #include guard doesn't match the file name
> > It originally was called `on`, thats why.
> Please, could you create an issue?
> >
> >> 22: "Also, if just a projection is given, then the projection will
> >> be called for each of its arguments."
> >> I don't see any good reason for this to be an
> >> overload of by. It's essentially a for_each.
> > It seems like a natural extension:
> >
> > by(p, f)(xs...) == f(p(xs)...)
> > by(p)(xs...) == p(xs)...;
> >
> >> 25: Note: All projections are always evaluated in order from
> left-to-right.
> >> By only takes one projection.
> > Yea, that should read all the applications of the projection.
> Please, could you create an issue?
> >> This is a lot of code to implement what is essentially a one-liner:
> >> apply_eval(f, capture_forward(x)(p)...)
> > It does that for older compilers, but on compilers that support it, it
> uses
> > initializers lists.
> What are the old compilers? I don't know if it will be worth to have a
> branch for compilers conforming to C++14 and one for others.
>
Well, MSVC and gcc 4.8 is included in the older compilers, but I can make
it work with Callable.
>
> >> concepts.md:
> >>
> >> Please reference the C++ standard when defining INVOKE.
> >> (Unless of course your INVOKE is different, which it
> >> had better not be)
> > Yes it is the same, I'll try to find the reference to the standard.
> >
> >
> Maybe you can reference
> http://en.cppreference.com/w/cpp/utility/functional/invoke
>
> There is already an issue for this point.
>
> Thanks again Steven.
>
> Even if you are not for the library in its current state your comments
> will help a lot to improve it, so please, could you reconsider to give
> us more feedback on other parts of the library?
>
Yes this feedback was very useful, and would appreciate more feedback as
well. I will open issues for the issues raised currently. More issues can
be raised directly on the github page.
Thanks,
Paul