FBString: fix constructors so it compiles with newer Clang's

Summary: Using Clang 3.7, this minimal test program:

  #include <folly/FBString.h>
  struct S { folly::basic_fbstring<char> x; };
  int main(int argc, char *argv[]) {
    S s {};
    return 0;
  }

... breaks with the following error output:

  FBStringTest.cpp:5:8: error: chosen constructor is explicit in copy-initialization
    S s {};
         ^
  ./folly/FBString.h:1009:12: note: constructor declared here
    explicit basic_fbstring(const A& = A()) noexcept {
             ^
  FBStringTest.cpp:3:40: note: in implicit initialization of field 'x' with omitted initializer
  struct S { folly::basic_fbstring<char> x; };

... because this `basic_fbstring` is used in a struct and the struct is being default-constructed.

This patch splits the (nevertheless-correct) one-default-arg constructor into two, to deconfuse clang and have it select the correct constructor to avoid this issue.

Reviewed By: matbd

Differential Revision: D2632953

fb-gh-sync-id: 2c75ae85732678c31543f5cccc73d58317982f07

diff --git a/folly/FBString.h b/folly/FBString.h
index 98bf1668e1e85c78601af9646445ba4e674d56bb..d8dccd8099f23fba27614bd9a94abd2f36a9ff1c 100644 (file)
--- a/folly/FBString.h
+++ b/folly/FBString.h
@@ -1006,7 +1006,23 @@ private:
 
 public:
   // C++11 21.4.2 construct/copy/destroy
-  explicit basic_fbstring(const A& /*a*/ = A()) noexcept {
+
+  // Note: while the following two constructors can be (and previously were)
+  // collapsed into one constructor written this way:
+  //
+  //   explicit basic_fbstring(const A& a = A()) noexcept { }
+  //
+  // This can cause Clang (at least version 3.7) to fail with the error:
+  //   "chosen constructor is explicit in copy-initialization ...
+  //   in implicit initialization of field '(x)' with omitted initializer"
+  //
+  // if used in a struct which is default-initialized.  Hence the split into
+  // these two separate constructors.
+
+  basic_fbstring() noexcept : basic_fbstring(A()) {
+  }
+
+  explicit basic_fbstring(const A&) noexcept {
   }
 
   basic_fbstring(const basic_fbstring& str)

diff --git a/folly/test/FBStringTest.cpp b/folly/test/FBStringTest.cpp
index e548a8d57dad69e4345acbc9198f4fe43218f0aa..15783ea1b6d60dd6d9b8af1dc074a566718dcf66 100644 (file)
--- a/folly/test/FBStringTest.cpp
+++ b/folly/test/FBStringTest.cpp
@@ -19,6 +19,7 @@
 
 #include <folly/FBString.h>
 
+#include <atomic>
 #include <cstdlib>
 
 #include <list>
@@ -1349,6 +1350,51 @@ TEST(FBString, moveTerminator) {
   EXPECT_EQ('\0', *s.c_str());
 }
 
+namespace {
+/*
+ * t8968589: Clang 3.7 refused to compile w/ certain constructors (specifically
+ * those that were "explicit" and had a defaulted parameter, if they were used
+ * in structs which were default-initialized).  Exercise these just to ensure
+ * they compile.
+ *
+ * In diff D2632953 the old constructor:
+ *   explicit basic_fbstring(const A& a = A()) noexcept;
+ *
+ * was split into these two, as a workaround:
+ *   basic_fbstring() noexcept;
+ *   explicit basic_fbstring(const A& a) noexcept;
+ */
+
+struct TestStructDefaultAllocator {
+  folly::basic_fbstring<char> stringMember;
+};
+
+template <class A>
+struct TestStructWithAllocator {
+  folly::basic_fbstring<char, std::char_traits<char>, A> stringMember;
+};
+
+std::atomic<size_t> allocatorConstructedCount(0);
+struct TestStructStringAllocator : std::allocator<char> {
+  TestStructStringAllocator() {
+    ++ allocatorConstructedCount;
+  }
+};
+
+}  // anon namespace
+
+TEST(FBStringCtorTest, DefaultInitStructDefaultAlloc) {
+  TestStructDefaultAllocator t1 { };
+  EXPECT_TRUE(t1.stringMember.empty());
+}
+
+TEST(FBStringCtorTest, DefaultInitStructAlloc) {
+  EXPECT_EQ(allocatorConstructedCount.load(), 0);
+  TestStructWithAllocator<TestStructStringAllocator> t2;
+  EXPECT_TRUE(t2.stringMember.empty());
+  EXPECT_EQ(allocatorConstructedCount.load(), 1);
+}
+
 int main(int argc, char** argv) {
   testing::InitGoogleTest(&argc, argv);
   gflags::ParseCommandLineFlags(&argc, &argv, true);