diff options
author | danakj <danakj@chromium.org> | 2015-12-07 16:44:41 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-12-08 00:46:50 +0000 |
commit | 314d1f44144e9d2a12d41445c9e5a1287e14373c (patch) | |
tree | c4ff7b8d1eaa817112400516ede35bda691c5fcd | |
parent | f981cb2a199f58143449eb64946b5d5e4d974187 (diff) | |
download | chromium_src-314d1f44144e9d2a12d41445c9e5a1287e14373c.zip chromium_src-314d1f44144e9d2a12d41445c9e5a1287e14373c.tar.gz chromium_src-314d1f44144e9d2a12d41445c9e5a1287e14373c.tar.bz2 |
base: Stop using Pass() on move-only types in Bind and Callback.
This replaces calls to .Pass() with calls to std::move() instead, which
will help allow us to remove the .Pass() method off of our move-only
types.
It does not dis/allow any new behaviour with Bind and Callback.
Specifically:
1. PassedWrapper is created from base::Passed. Previously PassedWrapper
called .Pass() which required a whitelisted type to compile. Now we get
a much nicer error by using enable_if on base::Passed to check that the
type is whitelisted:
../../base/callback_unittest.cc:221:66: error: no matching function for call to 'Passed'
Callback<void(void)> callback = base::Bind(&TakeUniqueByValue, base::Passed(&i));
^~~~~~~~~~~~
This prevents use such as base::Passed(&a_std_unique_ptr).
2. The CallbackParamTraits specialize on the whitelist, and for non-
whitelisted types it will store a T and pass a const T&. The receiving
method will take a T which will try to copy and fail for non-whitelisted
move-only types. Secondly, the constructor of BindState will fail as
Bind takes a const T& for each bound argument currently, and the storage
of T will try to copy into the BindState.
This prevents use such as base::Bind(&F, std::unique_ptr<T>()), which will
also apply to scoped_ptr.
R=Nico, dcheng
BUG=557422
Review URL: https://codereview.chromium.org/1496403002
Cr-Commit-Position: refs/heads/master@{#363635}
-rw-r--r-- | base/bind_helpers.h | 47 | ||||
-rw-r--r-- | base/callback_internal.h | 6 |
2 files changed, 33 insertions, 20 deletions
diff --git a/base/bind_helpers.h b/base/bind_helpers.h index 24063ad..434520a 100644 --- a/base/bind_helpers.h +++ b/base/bind_helpers.h @@ -111,7 +111,7 @@ // scoped_ptr<Foo> f(new Foo()); // // // |cb| is given ownership of Foo(). |f| is now NULL. -// // You can use f.Pass() in place of &f, but it's more verbose. +// // You can use std::move(f) in place of &f, but it's more verbose. // Closure cb = Bind(&TakesOwnership, Passed(&f)); // // // Run was never called so |cb| still owns Foo() and deletes @@ -143,6 +143,9 @@ #ifndef BASE_BIND_HELPERS_H_ #define BASE_BIND_HELPERS_H_ +#include <type_traits> +#include <utility> + #include "base/basictypes.h" #include "base/callback.h" #include "base/memory/weak_ptr.h" @@ -359,22 +362,24 @@ class OwnedWrapper { // created when we are explicitly trying to do a destructive move. // // Two notes: -// 1) PassedWrapper supports any type that has a "Pass()" function. -// This is intentional. The whitelisting of which specific types we -// support is maintained by CallbackParamTraits<>. +// 1) PassedWrapper supports any type that has a move constructor, however +// the type will need to be specifically whitelisted in order for it to be +// bound to a Callback. We guard this explicitly at the call of Passed() +// to make for clear errors. Things not given to Passed() will be forwarded +// and stored by value which will not work for general move-only types. // 2) is_valid_ is distinct from NULL because it is valid to bind a "NULL" // scoper to a Callback and allow the Callback to execute once. template <typename T> class PassedWrapper { public: - explicit PassedWrapper(T scoper) : is_valid_(true), scoper_(scoper.Pass()) {} + explicit PassedWrapper(T&& scoper) + : is_valid_(true), scoper_(std::move(scoper)) {} PassedWrapper(const PassedWrapper& other) - : is_valid_(other.is_valid_), scoper_(other.scoper_.Pass()) { - } + : is_valid_(other.is_valid_), scoper_(std::move(other.scoper_)) {} T Pass() const { CHECK(is_valid_); is_valid_ = false; - return scoper_.Pass(); + return std::move(scoper_); } private: @@ -566,17 +571,25 @@ static inline internal::OwnedWrapper<T> Owned(T* o) { return internal::OwnedWrapper<T>(o); } -// We offer 2 syntaxes for calling Passed(). The first takes a temporary and -// is best suited for use with the return value of a function. The second -// takes a pointer to the scoper and is just syntactic sugar to avoid having -// to write Passed(scoper.Pass()). -template <typename T> -static inline internal::PassedWrapper<T> Passed(T scoper) { - return internal::PassedWrapper<T>(scoper.Pass()); +// We offer 2 syntaxes for calling Passed(). The first takes an rvalue and +// is best suited for use with the return value of a function or other temporary +// rvalues. The second takes a pointer to the scoper and is just syntactic sugar +// to avoid having to write Passed(std::move(scoper)). +// +// Both versions of Passed() prevent T from being an lvalue reference. The first +// via use of enable_if, and the second takes a T* which will not bind to T&. +template <typename T, + typename std::enable_if<internal::IsMoveOnlyType<T>::value && + !std::is_lvalue_reference<T>::value>::type* = + nullptr> +static inline internal::PassedWrapper<T> Passed(T&& scoper) { + return internal::PassedWrapper<T>(std::move(scoper)); } -template <typename T> +template <typename T, + typename std::enable_if<internal::IsMoveOnlyType<T>::value>::type* = + nullptr> static inline internal::PassedWrapper<T> Passed(T* scoper) { - return internal::PassedWrapper<T>(scoper->Pass()); + return internal::PassedWrapper<T>(std::move(*scoper)); } template <typename T> diff --git a/base/callback_internal.h b/base/callback_internal.h index 8f0c2b3..aa8542c 100644 --- a/base/callback_internal.h +++ b/base/callback_internal.h @@ -93,7 +93,7 @@ class BASE_EXPORT CallbackBase { }; // A helper template to determine if given type is non-const move-only-type, -// i.e. if a value of the given type should be passed via .Pass() in a +// i.e. if a value of the given type should be passed via std::move() in a // destructive way. template <typename T> struct IsMoveOnlyType { template <typename U> @@ -197,7 +197,7 @@ struct CallbackParamTraitsForMoveOnlyType { // default template compiles out to be a no-op. // // In C++11, std::forward would replace all uses of this function. However, it -// is impossible to implement a general std::forward with C++11 due to a lack +// is impossible to implement a general std::forward without C++11 due to a lack // of rvalue references. // // In addition to Callback/Bind, this is used by PostTaskAndReplyWithResult to @@ -213,7 +213,7 @@ typename std::enable_if<!IsMoveOnlyType<T>::value, T>::type& CallbackForward( template <typename T> typename std::enable_if<IsMoveOnlyType<T>::value, T>::type CallbackForward( T& t) { - return t.Pass(); + return std::move(t); } } // namespace internal |