Boost logo

Boost :

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?
>> ???
> 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
> Emptiness is a correct argument. I probably should disable this warning.
Please, could you create an issue?
>> apply_eval.hpp:
>> 18: "Each [`eval`]( 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:
>> 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.
>> 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.
>> 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?


Boost list run by bdawes at, gregod at, cpdaniel at, john at