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