Update Bind to match its comments and always capture by value. Also update the generated count to 9 args.
The existing comment is wrong, and the test even ensures it: Bind will capture reference values by reference. That makes it hard to use with AsyncInvoker, because you can't safely Bind to a function that takes (const) reference params.
The new version of this code strips references in the bound object, so it captures by value, but can bind against functions that take const references, they'll just be references to the copy.
As the class comment implies, actual by-reference args should be passed as pointers or things that safely share (e.g. scoped_refptr) and not references directly. A new test case ensures the pointer reference works. The new code will also give a compiler error if you try to bind
to a non-const reference.
BUG=
Review URL: https://codereview.webrtc.org/1291543006
Cr-Commit-Position: refs/heads/master@{#10397}
diff --git a/webrtc/base/bind_unittest.cc b/webrtc/base/bind_unittest.cc
index fa47d27..be8d79c 100644
--- a/webrtc/base/bind_unittest.cc
+++ b/webrtc/base/bind_unittest.cc
@@ -25,7 +25,14 @@
int NullaryConst() const { ++call_count; return 2; }
void UnaryVoid(int dummy) { ++call_count; }
template <class T> T Identity(T value) { ++call_count; return value; }
- int UnaryByRef(int& value) const { ++call_count; return ++value; } // NOLINT
+ int UnaryByPointer(int* value) const {
+ ++call_count;
+ return ++(*value);
+ }
+ int UnaryByRef(const int& value) const {
+ ++call_count;
+ return ++const_cast<int&>(value);
+ }
int Multiply(int a, int b) const { ++call_count; return a * b; }
void RefArgument(const scoped_refptr<LifeTimeCheck>& object) {
EXPECT_TRUE(object.get() != nullptr);
@@ -64,26 +71,25 @@
// Try to catch any problem with scoped_refptr type deduction in rtc::Bind at
// compile time.
-static_assert(is_same<detail::RemoveScopedPtrRef<
- const scoped_refptr<RefCountInterface>&>::type,
- scoped_refptr<RefCountInterface>>::value,
- "const scoped_refptr& should be captured by value");
+static_assert(
+ is_same<
+ rtc::remove_reference<const scoped_refptr<RefCountInterface>&>::type,
+ const scoped_refptr<RefCountInterface>>::value,
+ "const scoped_refptr& should be captured by value");
-static_assert(is_same<detail::RemoveScopedPtrRef<const scoped_refptr<F>&>::type,
- scoped_refptr<F>>::value,
+static_assert(is_same<rtc::remove_reference<const scoped_refptr<F>&>::type,
+ const scoped_refptr<F>>::value,
"const scoped_refptr& should be captured by value");
static_assert(
- is_same<detail::RemoveScopedPtrRef<const int&>::type, const int&>::value,
- "const int& should be captured as const int&");
+ is_same<rtc::remove_reference<const int&>::type, const int>::value,
+ "const int& should be captured as const int");
-static_assert(
- is_same<detail::RemoveScopedPtrRef<const F&>::type, const F&>::value,
- "const F& should be captured as const F&");
+static_assert(is_same<rtc::remove_reference<const F&>::type, const F>::value,
+ "const F& should be captured as const F");
-static_assert(
- is_same<detail::RemoveScopedPtrRef<F&>::type, F&>::value,
- "F& should be captured as F&");
+static_assert(is_same<rtc::remove_reference<F&>::type, F>::value,
+ "F& should be captured as F");
#define EXPECT_IS_CAPTURED_AS_PTR(T) \
static_assert(is_same<detail::PointerType<T>::type, T*>::value, \
@@ -129,11 +135,20 @@
&object, string_value)());
EXPECT_EQ(6, object.call_count);
int value = 11;
- EXPECT_EQ(12, Bind(&MethodBindTester::UnaryByRef, &object, value)());
+ // Bind binds by value, even if the method signature is by reference, so
+ // "reference" binds require pointers.
+ EXPECT_EQ(12, Bind(&MethodBindTester::UnaryByPointer, &object, &value)());
EXPECT_EQ(12, value);
EXPECT_EQ(7, object.call_count);
- EXPECT_EQ(56, Bind(&MethodBindTester::Multiply, &object, 7, 8)());
+ // It's possible to bind to a function that takes a const reference, though
+ // the capture will be a copy. See UnaryByRef hackery above where it removes
+ // the const to make sure the underlying storage is, in fact, a copy.
+ EXPECT_EQ(13, Bind(&MethodBindTester::UnaryByRef, &object, value)());
+ // But the original value is unmodified.
+ EXPECT_EQ(12, value);
EXPECT_EQ(8, object.call_count);
+ EXPECT_EQ(56, Bind(&MethodBindTester::Multiply, &object, 7, 8)());
+ EXPECT_EQ(9, object.call_count);
}
TEST(BindTest, BindToFunction) {
@@ -210,13 +225,14 @@
} // anonymous namespace
-// Test Bind with non-scoped_refptr<> reference argument.
+// Test Bind with non-scoped_refptr<> reference argument, which should be
+// modified to a non-reference capture.
TEST(BindTest, RefArgument) {
const int x = 42;
- EXPECT_TRUE(Ref(x) == &x);
- // Bind() should not make a copy of |x|, i.e. the pointers should be the same.
+ EXPECT_EQ(&x, Ref(x));
+ // Bind() should make a copy of |x|, i.e. the pointers should be different.
auto functor = Bind(&Ref, x);
- EXPECT_TRUE(functor() == &x);
+ EXPECT_NE(&x, functor());
}
} // namespace rtc