base: Stop using Pass() on move-only types in Bind and Callback.

base/bind_helpers.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This defines a set of argument wrappers and related factory methods that
 // can be used specify the refcounting and reference semantics of arguments
 // that are bound by the Bind() function in base/bind.h.
 //
 // It also defines a set of simple functions and utilities that people want
 // when using Callback<> and Bind().
@@ skipping 93 matching lines @@
 //
 //
 // EXAMPLE OF Passed():
 //
 //   void TakesOwnership(scoped_ptr<Foo> arg) { }
 //   scoped_ptr<Foo> CreateFoo() { return scoped_ptr<Foo>(new Foo()); }
 //
 //   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
 //   // it on Reset().
 //   cb.Reset();
 //
 //   // |cb| is given a new Foo created by CreateFoo().
 //   cb = Bind(&TakesOwnership, Passed(CreateFoo()));
 //
 //   // |arg| in TakesOwnership() is given ownership of Foo(). |cb|
@@ skipping 11 matching lines @@
 //
 //   DoNothing() - Useful for creating a Closure that does nothing when called.
 //   DeletePointer<T>() - Useful for creating a Closure that will delete a
 //                        pointer when invoked. Only use this when necessary.
 //                        In most cases MessageLoop::DeleteSoon() is a better
 //                        fit.
 
 #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"
 #include "base/template_util.h"
 
 namespace base {
 namespace internal {
 
 // Use the Substitution Failure Is Not An Error (SFINAE) trick to inspect T
 // for the existence of AddRef() and Release() functions of the correct
@@ skipping 196 matching lines @@
 // the const correctness.
 //
 // This conundrum cannot be solved without either C++11 rvalue references or
 // a O(2^n) blowup of Bind() templates to handle each combination of regular
 // types and movable-but-not-copyable types.  Thus we introduce a wrapper type
 // that is copyable to transmit the correct type information down into
 // BindState<>. Ignoring const in this type makes sense because it is only
 // 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)

[danakj, 2015/12/04 20:46:13]

This take a T&&, but the T is a class template type, which means this is
strictly a T&&, not a universal reference.

This use of T&& prevents one move constructor from being called on Bind().

+      : 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:
   mutable bool is_valid_;
   mutable T scoper_;
 };
 
 // Unwrap the stored parameters for the wrappers above.
 template <typename T>
 struct UnwrapTraits {
@@ skipping 172 matching lines @@
 static inline internal::ConstRefWrapper<T> ConstRef(const T& o) {
   return internal::ConstRefWrapper<T>(o);
 }
 
 template <typename T>
 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());
+// 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(std::move(scoper)).
+//
+// Both versions of Passed() prevent T from being an lvalue reference. The first
+// via use of enable_if (to prevent reference collapsing from occuring), and the
+// second takes a T* which makes it illegal for T to be a reference.
+template <typename T,
+          typename std::enable_if<!std::is_lvalue_reference<T>::value &&
+                                  internal::IsMoveOnlyType<T>::value>::type* =
+              nullptr>
+static inline internal::PassedWrapper<T> Passed(T&& scoper) {

[danakj, 2015/12/04 20:46:13]

This one doesn't prevent any move constructors (I guess receiving by value into
a local and moving it in an inline method and all it compiles away). But I
thought it represents semantically what this is receiving better. WDYT?

[danakj, 2015/12/04 20:50:16]

I thought about this some more and I don't think I would ask other people to
write && in cases like this.. I'll remove this one.

[dcheng, 2015/12/04 20:59:08]

I think what you're seeing is copy elision. Is the compiler smart enough to
elide this if it's called as:
  base::Passed(std::move(x))
?

[danakj, 2015/12/04 21:05:25]

Oh, nope. It saves a move constructor to use T&& here in that case. Thanks.

+  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>
 static inline internal::IgnoreResultHelper<T> IgnoreResult(T data) {
   return internal::IgnoreResultHelper<T>(data);
 }
 
 template <typename T>
 static inline internal::IgnoreResultHelper<Callback<T> >
 IgnoreResult(const Callback<T>& data) {
   return internal::IgnoreResultHelper<Callback<T> >(data);
 }
 
 BASE_EXPORT void DoNothing();
 
 template<typename T>
 void DeletePointer(T* obj) {
   delete obj;
 }
 
 }  // namespace base
 
 #endif  // BASE_BIND_HELPERS_H_