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