Further improve sorted_vector_types standard compliance

Summary:
This is a follow up from D16427231; after testing the code in a similar situation on a mac (with `clang` and `libc++`) I realized there were still standards compliance issues.

In that diff, the guarantee we provide is:

```
 *   - insert() single key variants, emplace(), and emplace_hint() only provide
 *     the strong exception guarantee (unchanged when exception is thrown) when
 *     std::is_nothrow_move_constructible<value_type>::value is true.
```

The implementation is eventually backed by `std::vector`.

The guarantees provided by `std::vector` in the standard are slightly weaker (https://en.cppreference.com/w/cpp/container/vector/insert):

```
If an exception is thrown when inserting a single element at the end, and T is CopyInsertable or std::is_nothrow_move_constructible<T>::value is true, there are no effects (strong exception guarantee).
```

i.e. it only provides this safety guarantee if the element is inserted at the end, and that's exactly what the new test shows.

After discussion with nbronson we discovered that `libstdc++` and `libc++` are using different techniques to handle the corner case of `v.insert(0, v[1])` for `insert(iter, value_type const&)`. `libstdc++` makes the copy into a temporary, and then moves it in, so it does in fact provide a stronger guarantee than necessary. `libc++` checks the address of the reference it is given, and if it is in the moved range it adjusts it (!) and then uses the copy assignment operator. This means on copy assignment exception there will be a moved-from element at the desired index.

Section 26.2.1 para 11 says that `insert` or `emplace` will have no effects if an exception is thrown, unless otherwise specified. 26.3.11.5 is the `vector` modifiers, but that only mentions `insert`. That means we can fix this by merely switch our uses of `std::vector::insert` to `std::vector::emplace` inside `sorted_vector_map/set`. On `libstdc++` this should't have any effect, since both insert and emplace make a temporary copy for the non-end insertion case (in different but similar code paths). On `libc++` this will avoid the address-adjusting trick and use the temporary copy technique like in `libstdc++`.

Reviewed By: yfeldblum

Differential Revision: D20785383

fbshipit-source-id: 290c3c7c061dedf1660da9534f4b9cc338da6224

folly/sorted_vector_types.h

@@ -144,18 +144,18 @@ typename OurContainer::iterator insert_with_hint(
   if (hint == cont.end() || cmp(value, *hint)) {
     if (hint == cont.begin() || cmp(*(hint - 1), value)) {
       hint = po.increase_capacity(cont, hint);
-      return cont.insert(hint, std::forward<Value>(value));
+      return cont.emplace(hint, std::forward<Value>(value));
     } else {
-      return sorted.insert(std::forward<Value>(value)).first;
+      return sorted.emplace(std::forward<Value>(value)).first;
     }
   }
 
   if (cmp(*hint, value)) {
     if (hint + 1 == cont.end() || cmp(value, *(hint + 1))) {
       hint = po.increase_capacity(cont, hint + 1);
-      return cont.insert(hint, std::forward<Value>(value));
+      return cont.emplace(hint, std::forward<Value>(value));
     } else {
-      return sorted.insert(std::forward<Value>(value)).first;
+      return sorted.emplace(std::forward<Value>(value)).first;
     }
   }
 
@@ -459,7 +459,7 @@ class sorted_vector_set : detail::growth_policy_wrapper<GrowthPolicy> {
     iterator it = lower_bound(value);
     if (it == end() || value_comp()(value, *it)) {
       it = get_growth_policy().increase_capacity(m_.cont_, it);
-      return std::make_pair(m_.cont_.insert(it, value), true);
+      return std::make_pair(m_.cont_.emplace(it, value), true);
     }
     return std::make_pair(it, false);
   }
@@ -468,7 +468,7 @@ class sorted_vector_set : detail::growth_policy_wrapper<GrowthPolicy> {
     iterator it = lower_bound(value);
     if (it == end() || value_comp()(value, *it)) {
       it = get_growth_policy().increase_capacity(m_.cont_, it);
-      return std::make_pair(m_.cont_.insert(it, std::move(value)), true);
+      return std::make_pair(m_.cont_.emplace(it, std::move(value)), true);
     }
     return std::make_pair(it, false);
   }
@@ -986,7 +986,7 @@ class sorted_vector_map : detail::growth_policy_wrapper<GrowthPolicy> {
     iterator it = lower_bound(value.first);
     if (it == end() || value_comp()(value, *it)) {
       it = get_growth_policy().increase_capacity(m_.cont_, it);
-      return std::make_pair(m_.cont_.insert(it, value), true);
+      return std::make_pair(m_.cont_.emplace(it, value), true);
     }
     return std::make_pair(it, false);
   }
@@ -995,7 +995,7 @@ class sorted_vector_map : detail::growth_policy_wrapper<GrowthPolicy> {
     iterator it = lower_bound(value.first);
     if (it == end() || value_comp()(value, *it)) {
       it = get_growth_policy().increase_capacity(m_.cont_, it);
-      return std::make_pair(m_.cont_.insert(it, std::move(value)), true);
+      return std::make_pair(m_.cont_.emplace(it, std::move(value)), true);
     }
     return std::make_pair(it, false);
   }

folly/test/sorted_vector_test.cpp

@@ -85,6 +85,55 @@ struct CountCopyCtor {
 
 int CountCopyCtor::gCount_ = 0;
 
+struct KeyCopiedException : public std::exception {};
+/**
+ * Key that may throw on copy when throwOnCopy is set, but never on move.
+ * Use clone() to copy without throwing.
+ */
+struct KeyThatThrowsOnCopies {
+  int32_t key{};
+  bool throwOnCopy{};
+
+  KeyThatThrowsOnCopies() {}
+
+  /* implicit */ KeyThatThrowsOnCopies(int32_t key) noexcept
+      : key(key), throwOnCopy(false) {}
+  KeyThatThrowsOnCopies(int32_t key, bool throwOnCopy) noexcept
+      : key(key), throwOnCopy(throwOnCopy) {}
+
+  ~KeyThatThrowsOnCopies() noexcept {}
+
+  KeyThatThrowsOnCopies(KeyThatThrowsOnCopies const& other)
+      : key(other.key), throwOnCopy(other.throwOnCopy) {
+    if (throwOnCopy) {
+      throw KeyCopiedException{};
+    }
+  }
+
+  KeyThatThrowsOnCopies(KeyThatThrowsOnCopies&& other) noexcept = default;
+
+  KeyThatThrowsOnCopies& operator=(KeyThatThrowsOnCopies const& other) {
+    key = other.key;
+    throwOnCopy = other.throwOnCopy;
+    if (throwOnCopy) {
+      throw KeyCopiedException{};
+    }
+    return *this;
+  }
+
+  KeyThatThrowsOnCopies& operator=(KeyThatThrowsOnCopies&& other) noexcept =
+      default;
+
+  bool operator<(const KeyThatThrowsOnCopies& other) const {
+    return key < other.key;
+  }
+};
+
+static_assert(
+    std::is_nothrow_move_constructible<KeyThatThrowsOnCopies>::value &&
+        std::is_nothrow_move_assignable<KeyThatThrowsOnCopies>::value,
+    "non-noexcept move-constructible or move-assignable");
+
 } // namespace
 
 TEST(SortedVectorTypes, SetAssignmentInitListTest) {
@@ -862,6 +911,39 @@ TEST(SortedVectorTypes, TestEmplaceHint) {
   }
 }
 
+TEST(SortedVectorTypes, TestExceptionSafety) {
+  std::initializer_list<KeyThatThrowsOnCopies> const sortedUnique = {
+      0, 1, 4, 7, 9, 11, 15};
+  sorted_vector_set<KeyThatThrowsOnCopies> set = {sortedUnique};
+  EXPECT_EQ(set.size(), 7);
+
+  // Verify that we successfully insert when no exceptions are thrown.
+  KeyThatThrowsOnCopies key1(96, false);
+  auto hint1 = set.find(96);
+  set.insert(hint1, key1);
+  EXPECT_EQ(set.size(), 8);
+
+  // Verify that we don't add a key at the end if copying throws
+  KeyThatThrowsOnCopies key2(99, true);
+  auto hint2 = set.find(99);
+  try {
+    set.insert(hint2, key2);
+  } catch (const KeyCopiedException&) {
+    // swallow
+  }
+  EXPECT_EQ(set.size(), 8);
+
+  // Verify that we don't add a key in the middle if copying throws
+  KeyThatThrowsOnCopies key3(47, true);
+  auto hint3 = set.find(47);
+  try {
+    set.insert(hint3, key3);
+  } catch (const KeyCopiedException&) {
+    // swallow
+  }
+  EXPECT_EQ(set.size(), 8);
+}
+
 #if FOLLY_HAS_MEMORY_RESOURCE
 
 using folly::detail::std_pmr::memory_resource;