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

Subject: Re: [boost] [Fit] formal review starts today
From: paul Fultz (pfultz2_at_[hidden])
Date: 2016-03-03 21:16:04

> On Thursday, March 3, 2016 4:17 PM, Steven Watanabe <watanabesj_at_[hidden]> wrote:
> > AMDG
>
> On 03/03/2016 04:43 AM, Vicente J. Botet Escriba wrote:
>> Dear Boost community, sorry for the late announce
>>
>> The formal review of Paul Fultz II's Fit library starts today, 2nd
> March
>> and ends on 13th March.
>>
>
>>
>> We encourage your participation in this review. At a minimum, kindly state:
>> - Whether you believe the library should be accepted into Boost
>
> No, this library should not be accepted into Boost at this time.
>
>>   * Conditions for acceptance
>> - Your name
>
> Steven Watanabe
>
>> - Your knowledge of the problem domain.
>>
>
> High.
>
>> You are strongly encouraged to also provide additional information:
>> - What is your evaluation of the library's:
>>   * Design
>>   * Implementation
>>   * Documentation
>>   * Tests
>>   * Usefulness
>> - Did you attempt to use the library? If so:
>>   * Which compiler(s)
>>   * What was the experience? Any problems?
>
> msvc-12 (failed), msvc-14, clang-3.3 (failed), gcc-4.8.
> This is basically as expected.
>
>
>> - 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.

>   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.

>   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.

>
> - 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`.

> 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?

>   Even worse are the functions (e.g. capture)
>   which do not specify whether they require
>   const or not. 

That probably should be added.

> 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.
>
> - Please be pedantic about ADL.  If you aren't
>   it /will/ bite you.

Good point.

>
> Details:
>
> Please add .gitattributes
>
> test/Jamfile.v2:
> - 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.

> - [ test_all r ]
>   "r" is completely meaningless here.

That is the way it was from where I 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.

> 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

> 93: BOOST_FIT_UNARY_PERFECT_ID macro is undocumented and unused.

Oops, yea it should be deleted.

> 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.

> 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`).

> 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.

>
> 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.

> 61: #define BOOST_FIT_NO_CONSTEXPR_VOID 1
>     #ifndef BOOST_FIT_NO_CONSTEXPR_VOID
>     ???

Oops, that is a mistake.

> 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.

>
> 124: constexpr detail::always_base<void> operator()() const
>     This overload is not documented.  Should it be?

Yes, it is the case for always_void.

>
> 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.

>
> 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.

>
> 90: return eval_ordered<R>(f, pack_join(...), ...)
>     Watch out for ADL.

I need a Steven Watanabe compiler.

>
> 113: x(apply(f, BOOST_FIT_FORWARD(Ts)(xs)...), 0)
>     You need parens around the comma operator.
>     Also not tested.

For what?

>
> 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).

>
> 86: make_args_at(...)(..., make_perfect_ref(...)...)
>     ADL...
>
> 95: get_args<N>(...)
>     ADL
>
> 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.

> 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.

>
> 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.

>
> 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.

>
> 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.

Thanks for your feedback,
Paul