From 6435670bdccef08dcd2713ecbed1a57caa7abfad Mon Sep 17 00:00:00 2001
From: Eric Niebler <eniebler@fb.com>
Date: Thu, 11 May 2017 16:55:38 -0700
Subject: [PATCH] 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
---
 folly/Function.h               |  6 ++++-
 folly/test/FunctionRefTest.cpp | 47 ++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/folly/Function.h b/folly/Function.h
index 5f0d748c..263b0af0 100644
--- 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 dc7e3bdd..6ddf62ad 100644
--- 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; };
-- 
2.34.1