From 72ea700cfbbe54c1a3f30d7c9e61a2af9b8a7a1a Mon Sep 17 00:00:00 2001 From: Eric Niebler Date: Thu, 30 Mar 2017 14:09:32 -0700 Subject: [PATCH] Properly constrain folly::Function's generic conversion constructor and fix its noexcept specification Summary: The generic conversion constructor for `folly::Function` was not checking that the source object could successfully be copy/move constructed, leading to some `is_constructible` false positives. Also, the `noexcept` specification on the `Function` constructor wasn't taking into account that the source object might be copied into the Function, instead of moved. The copy could throw. Reviewed By: yfeldblum Differential Revision: D4775037 fbshipit-source-id: f337b41bf9ac431baa9457a501e63c18ca099e57 --- folly/Function.h | 109 ++++++++++++++++++---------- folly/test/FunctionTest.cpp | 138 ++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 39 deletions(-) diff --git a/folly/Function.h b/folly/Function.h index 307c15df..6c64647d 100644 --- a/folly/Function.h +++ b/folly/Function.h @@ -1,7 +1,5 @@ /* - * Copyright 2017 Facebook, Inc. - * - * @author Eric Niebler (eniebler@fb.com), Sven Over (over@fb.com) + * Copyright 2017-present Facebook, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -14,7 +12,9 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. - * + */ +/* + * @author Eric Niebler (eniebler@fb.com), Sven Over (over@fb.com) * Acknowledgements: Giuseppe Ottaviano (ott@fb.com) */ @@ -225,6 +225,7 @@ #include #include +#include namespace folly { @@ -246,14 +247,22 @@ union Data { }; template ::type> -using IsSmall = std::integral_constant< - bool, - (sizeof(FunT) <= sizeof(Data::tiny) && - // Same as is_nothrow_move_constructible, but w/ no template instantiation. - noexcept(FunT(std::declval())))>; +using IsSmall = Conjunction< + std::integral_constant, + std::is_nothrow_move_constructible>; using SmallTag = std::true_type; using HeapTag = std::false_type; +template +struct NotFunction : std::true_type {}; +template +struct NotFunction> : std::false_type {}; + +template ::type> +using DecayIfConstructible = typename std::enable_if< + Conjunction, std::is_constructible>::value, + FunT>::type; + struct CoerceTag {}; template @@ -351,7 +360,7 @@ struct FunctionTraits { return fn.call_(fn.data_, static_cast(args)...); } - struct SharedProxy { + class SharedProxy { std::shared_ptr> sp_; public: @@ -436,8 +445,6 @@ class Function final : private detail::function::FunctionTraits { template using IsSmall = detail::function::IsSmall; - using OtherSignature = typename Traits::OtherSignature; - // The `data_` member is mutable to allow `constCastFunction` to work without // invoking undefined behavior. Const-correctness is only violated when // `FunctionType` is a const function type (e.g., `int() const`) and `*this` @@ -449,7 +456,7 @@ class Function final : private detail::function::FunctionTraits { friend Traits; friend Function folly::constCastFunction<>( Function&&) noexcept; - friend class Function; + friend class Function; template Function(Fun&& fun, SmallTag) noexcept { @@ -469,7 +476,13 @@ class Function final : private detail::function::FunctionTraits { exec_ = &detail::function::execBig; } - Function(Function&& that, CoerceTag) noexcept { + template + Function(Function&& that, CoerceTag) + : Function(static_cast&&>(that), HeapTag{}) {} + + Function( + Function&& that, + CoerceTag) noexcept { that.exec_(Op::MOVE, &that.data_, &data_); std::swap(call_, that.call_); std::swap(exec_, that.exec_); @@ -482,13 +495,7 @@ class Function final : private detail::function::FunctionTraits { Function() = default; // not copyable - // NOTE: Deleting the non-const copy constructor is unusual but necessary to - // prevent copies from non-const `Function` object from selecting the - // perfect forwarding implicit converting constructor below - // (i.e., `template Function(Fun&&)`). - Function(Function&) = delete; Function(const Function&) = delete; - Function(const Function&&) = delete; /** * Move constructor @@ -505,32 +512,46 @@ class Function final : private detail::function::FunctionTraits { /* implicit */ Function(std::nullptr_t) noexcept {} /** - * Constructs a new `Function` from any callable object. This - * handles function pointers, pointers to static member functions, - * `std::reference_wrapper` objects, `std::function` objects, and arbitrary - * objects that implement `operator()` if the parameter signature - * matches (i.e. it returns R when called with Args...). - * For a `Function` with a const function type, the object must be - * callable from a const-reference, i.e. implement `operator() const`. - * For a `Function` with a non-const function type, the object will - * be called from a non-const reference, which means that it will execute - * a non-const `operator()` if it is defined, and falls back to - * `operator() const` otherwise. + * Constructs a new `Function` from any callable object that is _not_ a + * `folly::Function`. This handles function pointers, pointers to static + * member functions, `std::reference_wrapper` objects, `std::function` + * objects, and arbitrary objects that implement `operator()` if the parameter + * signature matches (i.e. it returns an object convertible to `R` when called + * with `Args...`). * * \note `typename = ResultOf` prevents this overload from being * selected by overload resolution when `fun` is not a compatible function. + * + * \note The noexcept requires some explanation. IsSmall is true when the + * decayed type fits within the internal buffer and is noexcept-movable. But + * this ctor might copy, not move. What we need here, if this ctor does a + * copy, is that this ctor be noexcept when the copy is noexcept. That is not + * checked in IsSmall, and shouldn't be, because once the Function is + * constructed, the contained object is never copied. This check is for this + * ctor only, in the case that this ctor does a copy. */ - template > - /* implicit */ Function(Fun&& fun) noexcept(IsSmall::value) + template < + typename Fun, + typename FunT = detail::function::DecayIfConstructible, + typename = typename Traits::template ResultOf> + /* implicit */ Function(Fun&& fun) noexcept( + IsSmall::value && noexcept(FunT(std::declval()))) : Function(static_cast(fun), IsSmall{}) {} /** - * For moving a `Function` into a `Function`. + * For move-constructing from a `folly::Function`. + * For a `Function` with a `const` function type, the object must be + * callable from a `const`-reference, i.e. implement `operator() const`. + * For a `Function` with a non-`const` function type, the object will + * be called from a non-const reference, which means that it will execute + * a non-const `operator()` if it is defined, and falls back to + * `operator() const` otherwise. */ template < - bool Const = Traits::IsConst::value, - typename std::enable_if::type = 0> - Function(Function&& that) noexcept + typename Signature, + typename = typename Traits::template ResultOf>> + Function(Function&& that) noexcept( + noexcept(Function(std::move(that), CoerceTag{}))) : Function(std::move(that), CoerceTag{}) {} /** @@ -553,7 +574,6 @@ class Function final : private detail::function::FunctionTraits { exec_(Op::NUKE, &data_, nullptr); } - Function& operator=(Function&) = delete; Function& operator=(const Function&) = delete; /** @@ -579,7 +599,7 @@ class Function final : private detail::function::FunctionTraits { * \note `typename = ResultOf` prevents this overload from being * selected by overload resolution when `fun` is not a compatible function. */ - template > + template ()))> Function& operator=(Fun&& fun) noexcept( noexcept(/* implicit */ Function(std::declval()))) { // Doing this in place is more efficient when we can do so safely. @@ -595,6 +615,17 @@ class Function final : private detail::function::FunctionTraits { return *this; } + /** + * For assigning from a `Function`. + */ + template < + typename Signature, + typename = typename Traits::template ResultOf>> + Function& operator=(Function&& that) noexcept( + noexcept(Function(std::move(that)))) { + return (*this = Function(std::move(that))); + } + /** * Clears this `Function`. */ diff --git a/folly/test/FunctionTest.cpp b/folly/test/FunctionTest.cpp index e9ac9430..3fda7cbe 100644 --- a/folly/test/FunctionTest.cpp +++ b/folly/test/FunctionTest.cpp @@ -54,8 +54,127 @@ struct Functor { template void deduceArgs(Function) {} +struct CallableButNotCopyable { + CallableButNotCopyable() {} + CallableButNotCopyable(CallableButNotCopyable const&) = delete; + CallableButNotCopyable(CallableButNotCopyable&&) = delete; + CallableButNotCopyable& operator=(CallableButNotCopyable const&) = delete; + CallableButNotCopyable& operator=(CallableButNotCopyable&&) = delete; + template + void operator()(Args&&...) const {} +}; + } // namespace +// TEST ===================================================================== +// Test constructibility and non-constructibility for some tricky conversions +static_assert( + !std::is_assignable, CallableButNotCopyable>::value, + ""); +static_assert( + !std::is_constructible, CallableButNotCopyable&>::value, + ""); +static_assert( + !std::is_constructible, CallableButNotCopyable>:: + value, + ""); +static_assert( + !std::is_constructible, CallableButNotCopyable&>:: + value, + ""); + +static_assert( + !std::is_assignable, CallableButNotCopyable>::value, + ""); +static_assert( + !std::is_assignable, CallableButNotCopyable&>::value, + ""); +static_assert( + !std::is_assignable, CallableButNotCopyable>::value, + ""); +static_assert( + !std::is_assignable, CallableButNotCopyable&>::value, + ""); + +static_assert( + std::is_constructible, Function>::value, + ""); +static_assert( + !std::is_constructible, Function>::value, + ""); +static_assert( + std::is_constructible, Function>:: + value, + ""); +static_assert( + !std::is_constructible, Function>:: + value, + ""); +static_assert( + !std::is_constructible, Function&>:: + value, + ""); +static_assert( + !std::is_constructible, Function&>:: + value, + ""); +static_assert( + !std::is_constructible, Function&>:: + value, + ""); +static_assert( + !std::is_constructible, Function&>:: + value, + ""); + +static_assert( + std::is_assignable, Function>::value, + ""); +static_assert( + !std::is_assignable, Function>::value, + ""); +static_assert( + std::is_assignable, Function>::value, + ""); +static_assert( + !std::is_assignable, Function>:: + value, + ""); +static_assert( + !std::is_assignable, Function&>::value, + ""); +static_assert( + !std::is_assignable, Function&>::value, + ""); +static_assert( + !std::is_assignable, Function&>:: + value, + ""); +static_assert( + !std::is_assignable, Function&>:: + value, + ""); + +static_assert( + std::is_nothrow_constructible< + Function, + Function>::value, + ""); +static_assert( + !std::is_nothrow_constructible< + Function, + Function>::value, + ""); +static_assert( + std::is_nothrow_assignable, Function>:: + value, + ""); +static_assert( + !std::is_nothrow_assignable< + Function, + Function>::value, + ""); + // TEST ===================================================================== // InvokeFunctor & InvokeReference @@ -938,3 +1057,22 @@ TEST(Function, DeducableArguments) { deduceArgs(Function{[](int, float) {}}); deduceArgs(Function{[](int i, float) { return i; }}); } + +TEST(Function, CtorWithCopy) { + struct X { + X() {} + X(X const&) noexcept(true) {} + X& operator=(X const&) = default; + }; + struct Y { + Y() {} + Y(Y const&) noexcept(false) {} + Y(Y&&) noexcept(true) {} + Y& operator=(Y&&) = default; + Y& operator=(Y const&) = default; + }; + auto lx = [x = X()]{}; + auto ly = [y = Y()]{}; + EXPECT_TRUE(noexcept(Function(lx))); + EXPECT_FALSE(noexcept(Function(ly))); +} -- 2.34.1