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