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

Subject: Re: [boost] [Fit] formal review starts today
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2016-03-03 17:16:32

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,
  placeholders <-> lambda, bind, phoenix
  Various configuration macros <-> Boost.Config

- The library attempts to take advantage of EBO,
  but makes several mistakes.  (See attached tests
  which fail to compile)

- 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.  It's especially
  weird when a function (e.g. partial) requires
  both ConstCallable, and MoveConstructible.
  Even worse are the functions (e.g. capture)
  which do not specify whether they require
  const or not.  You should not be requiring
  const at all, for anything.  See [func.bind.bind]
  for an interface that handles const correctly.

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

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)
- [ test_all r ]
  "r" is completely meaningless here.
- 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."

alias.hpp:
57: #pragma warning(disable: 4579)
    Library headers *must* disable warnings inside push/pop.
67: std::false_type is defined somewhere... (It looks like
    you get <type_traits> via delegate.hpp)
74: Ditto for std::is_same
93: BOOST_FIT_UNARY_PERFECT_ID macro is undocumented and unused.
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.
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.
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?

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?
61: #define BOOST_FIT_NO_CONSTEXPR_VOID 1
    #ifndef BOOST_FIT_NO_CONSTEXPR_VOID
    ???
121: always_base has boost::fit::detail as an
     associated namespace.

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

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'

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.

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

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

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.

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

25: Note: All projections are always evaluated in order from left-to-right.
    By only takes one projection.

This is a lot of code to implement what is essentially a one-liner:
  apply_eval(f, capture_forward(x)(p)...)

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.

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.

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.

placeholders.hpp:

We really don't need another lambda library, let alone
a half-baked one.

concepts.md:

Please reference the C++ standard when defining INVOKE.
(Unless of course your INVOKE is different, which it
had better not be)

In Christ,
Steven Watanabe