Actually mark Unexpected as cold

Summary:
Testing indicates that GCC ignores the cold attribute when the function
is available for inlining. Because Unexpected is a template class we
can't make the constructors non-inline, but we can make it derive from a
class with a cold constructor, which has the effect of making all the
Unexpected constructors implicitly cold.

Reviewed By: yfeldblum

Differential Revision: D6261013

fbshipit-source-id: 482e49253d5b104742018133c53fb60279dd9f9b

diff --git a/folly/Expected.h b/folly/Expected.h
index 2705f33533e7e95fc43b191b9611f74e9f9f422e..dcdb378e727acf2b3fb6d6b5fee72a933641f3d9 100644 (file)
--- a/folly/Expected.h
+++ b/folly/Expected.h
@@ -39,6 +39,7 @@
 #include <folly/Traits.h>
 #include <folly/Unit.h>
 #include <folly/Utility.h>
+#include <folly/lang/ColdClass.h>
 
 #define FOLLY_EXPECTED_ID(X) FB_CONCATENATE(FB_CONCATENATE(Folly, X), __LINE__)
 
@@ -675,7 +676,7 @@ namespace expected_detail {
  * Expected objects in the error state.
  */
 template <class Error>
-class Unexpected final {
+class Unexpected final : ColdClass {
   template <class E>
   friend class Unexpected;
   template <class V, class E>
@@ -712,10 +713,8 @@ class Unexpected final {
   Unexpected(Unexpected&&) = default;
   Unexpected& operator=(const Unexpected&) = default;
   Unexpected& operator=(Unexpected&&) = default;
-  FOLLY_COLD constexpr /* implicit */ Unexpected(const Error& err)
-      : error_(err) {}
-  FOLLY_COLD constexpr /* implicit */ Unexpected(Error&& err)
-      : error_(std::move(err)) {}
+  constexpr /* implicit */ Unexpected(const Error& err) : error_(err) {}
+  constexpr /* implicit */ Unexpected(Error&& err) : error_(std::move(err)) {}
 
   template <class Other FOLLY_REQUIRES_TRAILING(
       std::is_constructible<Error, Other&&>::value)>

diff --git a/folly/Makefile.am b/folly/Makefile.am
index 422b7bb872f7416f060617e387714d5875549cfa..83f79c20e3f736d3689bc9caa4dd674b59a1675d 100644 (file)
--- a/folly/Makefile.am
+++ b/folly/Makefile.am
@@ -313,6 +313,7 @@ nobase_follyinclude_HEADERS = \
        io/async/test/Util.h \
        json.h \
        lang/Assume.h \
+       lang/ColdClass.h \
        lang/Launder.h \
        lang/RValueReferenceWrapper.h \
        lang/SafeAssert.h \
@@ -566,6 +567,7 @@ libfolly_la_SOURCES = \
        io/async/ssl/SSLErrors.cpp \
        json.cpp \
        lang/Assume.cpp \
+       lang/ColdClass.cpp \
        lang/SafeAssert.cpp \
        detail/MemoryIdler.cpp \
        detail/SocketFastOpen.cpp \

diff --git a/folly/lang/ColdClass.cpp b/folly/lang/ColdClass.cpp
new file mode 100644 (file)
index 0000000..cd1827f
--- /dev/null
+++ b/folly/lang/ColdClass.cpp
@@ -0,0 +1,18 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "ColdClass.h"
+
+folly::cold_detail::ColdClass::ColdClass() noexcept {}

diff --git a/folly/lang/ColdClass.h b/folly/lang/ColdClass.h
new file mode 100644 (file)
index 0000000..25d266f
--- /dev/null
+++ b/folly/lang/ColdClass.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/* Tag any class as `cold` by inheriting from folly::cold::ColdClass
+ *
+ * Intended use: things like folly::Unexpected which are designed to only be
+ * instantiated on error paths.
+ */
+#pragma once
+
+#include <folly/CppAttributes.h>
+
+namespace folly {
+// ColdClass should be in its own namespace: inheriting from any class adds its
+// innermost namespace to the namespaces inspected during
+// argument-dependent-lookoup. We want people to be able to derive from this
+// without implicitly picking up the folly namespace for ADL on their classes.
+namespace cold_detail {
+struct ColdClass {
+  FOLLY_COLD ColdClass() noexcept;
+};
+} // namespace cold
+
+/* using override */ using cold_detail::ColdClass;
+} // namespace folly

diff --git a/folly/lang/test/ColdClassTest.cpp b/folly/lang/test/ColdClassTest.cpp
new file mode 100644 (file)
index 0000000..30d70ce
--- /dev/null
+++ b/folly/lang/test/ColdClassTest.cpp
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <folly/lang/ColdClass.h>
+
+#include <folly/portability/GTest.h>
+#include <type_traits>
+
+using folly::ColdClass;
+
+TEST(ColdClass, inheritance) {
+  // The only verifiable property of ColdClass is that it must not disrupt the
+  // default constructor/destructor, default copy/move constructors and default
+  // copy/move assignment operators when a class derives from it.
+  struct TestStruct : ColdClass {};
+  EXPECT_TRUE(std::is_nothrow_default_constructible<TestStruct>::value);
+  EXPECT_TRUE(std::is_trivially_copy_constructible<TestStruct>::value);
+  EXPECT_TRUE(std::is_trivially_move_constructible<TestStruct>::value);
+  EXPECT_TRUE(std::is_trivially_copy_assignable<TestStruct>::value);
+  EXPECT_TRUE(std::is_trivially_move_assignable<TestStruct>::value);
+  EXPECT_TRUE(std::is_trivially_destructible<TestStruct>::value);
+  // Same again, but private inheritance. Should make no difference.
+  class TestClass : ColdClass {};
+  EXPECT_TRUE(std::is_nothrow_default_constructible<TestClass>::value);
+  EXPECT_TRUE(std::is_trivially_copy_constructible<TestClass>::value);
+  EXPECT_TRUE(std::is_trivially_move_constructible<TestClass>::value);
+  EXPECT_TRUE(std::is_trivially_copy_assignable<TestClass>::value);
+  EXPECT_TRUE(std::is_trivially_move_assignable<TestClass>::value);
+  EXPECT_TRUE(std::is_trivially_destructible<TestClass>::value);
+}