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...@gmail.com <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