Copying a non-const FunctionRef lvalue should be a trivial operation

Summary: Before this change, when a non-const FunctionRef lvalue is copied, it is treated as any other callable: it is wrapped with an indirection. That's semantically incorrect and potentially creates lifetime problems. Instead, use the compiler generated copy constructor, which only copies the object and function pointers.

Reviewed By: yfeldblum

Differential Revision: D5040843

fbshipit-source-id: f691060bdced2e287ba22d22b961c02c2b924147

diff --git a/folly/Function.h b/folly/Function.h
index 5f0d748cf5656f90ea7a242d47572d9b7aa5b6ce..263b0af07d8ac940b186038dd9bbd5c9e4b436df 100644 (file)
--- a/folly/Function.h
+++ b/folly/Function.h
@@ -791,7 +791,11 @@ class FunctionRef<ReturnType(Args...)> final {
   /**
    * Construct a FunctionRef from a reference to a callable object.
    */
-  template <typename Fun>
+  template <
+      typename Fun,
+      typename std::enable_if<
+          !std::is_same<FunctionRef, typename std::decay<Fun>::type>::value,
+          int>::type = 0>
   /* implicit */ FunctionRef(Fun&& fun) noexcept {
     using ReferencedType = typename std::remove_reference<Fun>::type;
 

diff --git a/folly/test/FunctionRefTest.cpp b/folly/test/FunctionRefTest.cpp
index dc7e3bdd097638bf28f2e25962504412f819d9ec..6ddf62ad44e1c4cf4ffa9f58c41468df6d9926f4 100644 (file)
--- a/folly/test/FunctionRefTest.cpp
+++ b/folly/test/FunctionRefTest.cpp
@@ -22,6 +22,53 @@
 using folly::Function;
 using folly::FunctionRef;
 
+TEST(FunctionRef, Traits) {
+  static_assert(std::is_literal_type<FunctionRef<int(int)>>::value, "");
+// Some earlier versions of libstdc++ lack these traits. Frustrating that
+// the value of __GLIBCXX__ doesn't increase with version, but rather reflects
+// release date, so some larger values of __GLIBCXX__ lack the traits while
+// some smaller values have them. Can't figure out how to reliably test for the
+// presence or absence of the traits. :-(
+#if !defined(__GLIBCXX__) || __GNUC__ >= 5
+  static_assert(
+      std::is_trivially_copy_constructible<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_trivially_move_constructible<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_trivially_constructible<
+          FunctionRef<int(int)>,
+          FunctionRef<int(int)>&>::value,
+      "");
+  static_assert(
+      std::is_trivially_copy_assignable<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_trivially_move_assignable<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_trivially_assignable<
+          FunctionRef<int(int)>,
+          FunctionRef<int(int)>&>::value,
+      "");
+#endif
+  static_assert(
+      std::is_nothrow_copy_constructible<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_nothrow_move_constructible<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_nothrow_constructible<
+          FunctionRef<int(int)>,
+          FunctionRef<int(int)>&>::value,
+      "");
+  static_assert(
+      std::is_nothrow_copy_assignable<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_nothrow_move_assignable<FunctionRef<int(int)>>::value, "");
+  static_assert(
+      std::is_nothrow_assignable<
+          FunctionRef<int(int)>,
+          FunctionRef<int(int)>&>::value,
+      "");
+}
+
 TEST(FunctionRef, Simple) {
   int x = 1000;
   auto lambda = [&x](int v) { return x += v; };