Re: [boost] [Fit]Â formal review starts today

Subject: Re: [boost] [Fit] formal review starts today
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2016-03-04 13:55:33

Le 04/03/2016 03:16, paul Fultz a écrit :
>> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watanabesj_at_[hidden]> 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?
>>    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.
>
>>    Various configuration macros <-> Boost.Config
>>
>> - The library attempts to take advantage of EBO,
>>    but makes several mistakes.  (See attached tests
>>    which fail to compile)
> Oops, this only affects compressed_pair(and is an easy fix), `pack` does not
> have this issue.
Please, could you create an issue and tag it review?
>
>> - 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?
>
>> It's especially
>>    weird when a function (e.g. partial) requires
>>    both ConstCallable, and MoveConstructible.
> Almost all of the function adaptor require that. Why is that weird?
Why do you need that the Callable must be a ConstCallable?
>
>>    Even worse are the functions (e.g. capture)
>>    which do not specify whether they require
>>    const or not.
> That probably should be added.
Please, could you create an issue?
>
>> You should not be requiring
>>    const at all, for anything.  See [func.bind.bind]
>>    for an interface that handles const correctly.
> I probably should add Q/A for this. I make mutability explicit because for
> most cases `std::ref` should be used when using a mutable function object. If
> its really necessary(for example working with a third-party library) then
> `mutable_` adaptor can be used.
>
>> - The implementation has various useless bits,
>>    which serve no purpose that I can discern.
Steven, could you be more explicit?
>>
>> - Please be pedantic about ADL.  If you aren't
>>    it /will/ bite you.
> Good point.
Please, could you create an issue?
>
>> Details:
>>
>> Please add .gitattributes
Please, could you create an issue?
>>
>> test/Jamfile.v2:
Steven, Paul added the Jamfiles just before the review. There is not a 
lot of people that master BJam :(
>> - project fit
>>    Project names must be globally unique, so it's
>>    better to use /boost/fit (or leave out the
>>    name entirely)
> I didn't realize I could do that.
Please, could you create an issue?
>
>> - [ test_all r ]
>>    "r" is completely meaningless here.
> That is the way it was from where I copied it.
 From where you copied it?
>
>> - Actually, everything from rule test-all
>>    down can be reduced to:
>>    for local fileb in [ glob *.cpp ]
>>    {
>>      run $(fileb) ;
>>    }
>>    You don't need to create a special target
>>    to group the tests, unless you want more
>>    granularity than "run everything" or "run
>>    specific tests by name."
> Interesting. I really could find no reference on this information.
>
>> alias.hpp:
>> 57: #pragma warning(disable: 4579)
>>      Library headers *must* disable warnings inside push/pop.
> Noted.
Please, could you create an issue?
>> 67: std::false_type is defined somewhere... (It looks like
>>      you get <type_traits> via delegate.hpp)
>> 74: Ditto for std::is_same
> Yes, from delegate.hpp
What Steven wants is that the dependencies be more explicit. If you use 
some traits you should include <type_traits>.
Please, could you create an issue?
>
>> 93: BOOST_FIT_UNARY_PERFECT_ID macro is undocumented and unused.
> Oops, yea it should be deleted.
Please, could you create an issue?
>
>> 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?
Please, could you create an issue?
>
>> 117: struct alias_inherit
>>      #if (defined(__GNUC__) && !defined (__clang__))
>>      : std::conditional<(std::is_class<T>::value), T,
>> alias<T>>::type
>>      Really?  alias_inherit is documented to use
>>      inheritence, so it should be an error to use
>>      it with a non-class type.
> Yes it is an error to use something other than a class. This is to avoid a
> problem on gcc when calling `alias_value` that eagerly instantiates
> `alias_inherit`(even though I have an `enable_if` for `alias_value`).
Could this be documented in the code?
>
>> 145: // Since we disable the error for 4579 on MSVC, which leaves the static
>>      // member unitialized at runtime, it is, therefore, only safe to
>> use this
>>      // class on types that are empty with constructors that have no
>> possible
>>      // side effects.
>>      Shouldn't it also be safe for types with trivial
>>      default constructors, since then, zero-initialization
>>      will do the right thing?
> That is probably another case as well.
Please, could you create an issue?
>
>> always.hpp:
>>
>> 22: "The `always_ref` version will return a reference, and it
>>      requires the value passed in to be an lvalue."
>>      The first time I read this I thought it meant
>>      that the function object created by always_ref
>>      would be a reference, not that it produces
>>      a reference when called.  Consider rephrasing
>>      the sentence to avoid the ambiguity.
>> 39: "Requirements: T must be: * CopyConstructible"
>>      This only applies to always, not always_ref, no?
> Yes.
Please, could you create an issue?
>
>> 61: #define BOOST_FIT_NO_CONSTEXPR_VOID 1
>>      #ifndef BOOST_FIT_NO_CONSTEXPR_VOID
>>      ???
> Oops, that is a mistake.
Please, could you create an issue?
>
>> 121: always_base has boost::fit::detail as an
>>      associated namespace.
> Ah, I see, this could have the potential for strange ADL lookup. I'll probably
> need to move this stuff to their own namespace.
Please, could you create an issue?
>
>> 124: constexpr detail::always_base<void> operator()() const
>>      This overload is not documented.  Should it be?
> Yes, it is the case for always_void.
Please, could you create an issue?
>
>> apply.hpp:
>> 29: assert(compress(apply, f)(x, y, z) == f(x)(y)(z));
>>      I don't think that compress should be part of the
>>      semantics of apply.  This belongs as an example.
>>
>> msvc-14.0 says:
>> boost/fit/apply.hpp(83): warning C4003: not enough actual parameters for
>> macro 'BOOST_FIT_APPLY_MEM_FN_CALL'
> Emptiness is a correct argument. I probably should disable this warning.
Please, could you create an issue?
>
>> apply_eval.hpp:
>>
>> 18: "Each [`eval`](eval.md) call is always ordered
>>      from left-to-right."
>>      It isn't clear to me why this guarantee is important.
>>      Not to mention that the test case apply_eval.cpp
>>      doesn't check it.
> That is the whole purpose of apply_eval, so the user can order the parameters
> that are applied to a function.
What about the test?
>
>> 90: return eval_ordered<R>(f, pack_join(...), ...)
>>      Watch out for ADL.
> I need a Steven Watanabe compiler.
Please, could you create an issue?
>
>> 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0)
>>      You need parens around the comma operator.
>>      Also not tested.
> For what?
What about the test?
>
>> 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?
>
>> 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.
>
>> 135: #define BOOST_FIT_BY_VOID_RETURN BOOST_FIT_ALWAYS_VOID_RETURN
>> 138: #define BOOST_FIT_BY_VOID_RETURN boost::fit::detail::swallow
>>      You should really have a single BOOST_FIT_VOID_RETURN
>>      and use it consistently everywhere.
> The BOOST_FIT_BY_VOID_RETURN is only used in one place so it could be replaced
> with preprocessor ifs.
Please, could you create an issue?
>
>> capture.hpp:
>>
>> 32: // Capture lvalues by reference and rvalues by value.
>>      The default should be to capture everything by value.
>>      Capturing by reference is potentially dangerous, and
>>      switching based on whether the argument is an lvalue
>>      is even more so.
> I don't see how it makes it more dangerous. Everything in scope is captured by
> reference and temporaries have there lifetimes extened so they will remain in
> scope as well. This works great when forwarding is added on.
>
>> 79: return always_ref(*this)(xs...);
>>      Why not return *this?
>>      The always_ref dance serves no purpose,
>>      other than making the code hard to follow.
> This is necessary because of the eagerness of constexpr.
>
>> placeholders.hpp:
>>
>> We really don't need another lambda library, let alone
>> a half-baked one.
> Its not designed to be a full lambda library. Just enough to handle some
> constexpr cases.
>
>> 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?

Best,
Vicente