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
> Le 04/03/2016 03:16, paul Fultz a Ã©crit :
> >> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watan..._at_[hidden]
> >>> 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
> > support constexpr. Plus, I structured it in a way to avoid bugs with gcc
> > 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
> > 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
> > 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
> > 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
> > future, I may make this a template variable(so `arg_c<5>` will be a
> > 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
> >> 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
> > 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
> 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk