Improve sorted_vector_types standard compliance

Summary:
Note that we only provide the strong exception guarantee for single key insertion and emplacement when `std::is_nothrow_constructible<value_type>::value == true`. We could `static_assert()` that in `sorted_vector_set`, since it seems to hold for all contbuilds. But for `sorted_vector_map` there are quite a few contbuilds broken by that static assert. So I've chosen not to static assert in `sorted_vector_set` for consistency.

`insert()`, `emplace*()` and `erase()` take `const_iterator` in compliance with C++11. `erase()` has an overload that takes an `iterator` in C++17, but since we are backed by a vector I don't think that is necessary.

Add `emplace_hint()` variants.

Reviewed By: yfeldblum

Differential Revision: D16427231

fbshipit-source-id: 4bcf6fd01d9b1320548d12b152d1cef9291c2dd2

folly/sorted_vector_types.h

@@ -56,6 +56,9 @@
  *   - sorted_vector_map::value_type is pair<K,V>, not pair<const K,V>.
  *     (This is basically because we want to store the value_type in
  *     std::vector<>, which requires it to be Assignable.)
+ *   - 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.
  */
 
 #pragma once
@@ -131,7 +134,7 @@ template <class OurContainer, class Vector, class GrowthPolicy>
 typename OurContainer::iterator insert_with_hint(
     OurContainer& sorted,
     Vector& cont,
-    typename OurContainer::iterator hint,
+    typename OurContainer::const_iterator hint,
     typename OurContainer::value_type&& value,
     GrowthPolicy& po) {
   const typename OurContainer::value_compare& cmp(sorted.value_comp());
@@ -154,7 +157,7 @@ typename OurContainer::iterator insert_with_hint(
   }
 
   // Value and *hint did not compare, so they are equal keys.
-  return hint;
+  return sorted.begin() + std::distance(sorted.cbegin(), hint);
 }
 
 template <class OurContainer, class Vector, class InputIterator>
@@ -423,11 +426,11 @@ class sorted_vector_set : detail::growth_policy_wrapper<GrowthPolicy> {
     return std::make_pair(it, false);
   }
 
-  iterator insert(iterator hint, const value_type& value) {
+  iterator insert(const_iterator hint, const value_type& value) {
     return insert(hint, std::move(value_type(value)));
   }
 
-  iterator insert(iterator hint, value_type&& value) {
+  iterator insert(const_iterator hint, value_type&& value) {
     return detail::insert_with_hint(
         *this, m_.cont_, hint, std::move(value), get_growth_policy());
   }
@@ -457,6 +460,22 @@ class sorted_vector_set : detail::growth_policy_wrapper<GrowthPolicy> {
     return insert(std::move(value));
   }
 
+  // emplace_hint isn't better than insert for sorted_vector_set, but aids
+  // compatibility
+  template <typename... Args>
+  iterator emplace_hint(const_iterator hint, Args&&... args) {
+    value_type v(std::forward<Args>(args)...);
+    return insert(hint, std::move(v));
+  }
+
+  iterator emplace_hint(const_iterator hint, const value_type& value) {
+    return insert(hint, value);
+  }
+
+  iterator emplace_hint(const_iterator hint, value_type&& value) {
+    return insert(hint, std::move(value));
+  }
+
   size_type erase(const key_type& key) {
     iterator it = find(key);
     if (it == end()) {
@@ -466,11 +485,11 @@ class sorted_vector_set : detail::growth_policy_wrapper<GrowthPolicy> {
     return 1;
   }
 
-  iterator erase(iterator it) {
+  iterator erase(const_iterator it) {
     return m_.cont_.erase(it);
   }
 
-  iterator erase(iterator first, iterator last) {
+  iterator erase(const_iterator first, const_iterator last) {
     return m_.cont_.erase(first, last);
   }
 
@@ -837,11 +856,11 @@ class sorted_vector_map : detail::growth_policy_wrapper<GrowthPolicy> {
     return std::make_pair(it, false);
   }
 
-  iterator insert(iterator hint, const value_type& value) {
+  iterator insert(const_iterator hint, const value_type& value) {
     return insert(hint, std::move(value_type(value)));
   }
 
-  iterator insert(iterator hint, value_type&& value) {
+  iterator insert(const_iterator hint, value_type&& value) {
     return detail::insert_with_hint(
         *this, m_.cont_, hint, std::move(value), get_growth_policy());
   }
@@ -871,6 +890,22 @@ class sorted_vector_map : detail::growth_policy_wrapper<GrowthPolicy> {
     return insert(std::move(value));
   }
 
+  // emplace_hint isn't better than insert for sorted_vector_set, but aids
+  // compatibility
+  template <typename... Args>
+  iterator emplace_hint(const_iterator hint, Args&&... args) {
+    value_type v(std::forward<Args>(args)...);
+    return insert(hint, std::move(v));
+  }
+
+  iterator emplace_hint(const_iterator hint, const value_type& value) {
+    return insert(hint, value);
+  }
+
+  iterator emplace_hint(const_iterator hint, value_type&& value) {
+    return insert(hint, std::move(value));
+  }
+
   size_type erase(const key_type& key) {
     iterator it = find(key);
     if (it == end()) {
@@ -880,11 +915,11 @@ class sorted_vector_map : detail::growth_policy_wrapper<GrowthPolicy> {
     return 1;
   }
 
-  iterator erase(iterator it) {
+  iterator erase(const_iterator it) {
     return m_.cont_.erase(it);
   }
 
-  iterator erase(iterator first, iterator last) {
+  iterator erase(const_iterator first, const_iterator last) {
     return m_.cont_.erase(first, last);
   }
 

folly/test/sorted_vector_test.cpp

@@ -22,6 +22,7 @@
 #include <string>
 
 #include <folly/Range.h>
+#include <folly/Utility.h>
 #include <folly/portability/GMock.h>
 #include <folly/portability/GTest.h>
 
@@ -65,7 +66,8 @@ struct CountCopyCtor {
 
   explicit CountCopyCtor(int val) : val_(val), count_(0) {}
 
-  CountCopyCtor(const CountCopyCtor& c) : val_(c.val_), count_(c.count_ + 1) {}
+  CountCopyCtor(const CountCopyCtor& c) noexcept
+      : val_(c.val_), count_(c.count_ + 1) {}
 
   bool operator<(const CountCopyCtor& o) const {
     return val_ < o.val_;
@@ -846,3 +848,29 @@ TEST(SortedVectorTypes, TestDataPointsToFirstElement) {
   EXPECT_EQ(set.data(), &*set.begin());
   EXPECT_EQ(map.data(), &*map.begin());
 }
+
+TEST(SortedVectorTypes, TestEmplaceHint) {
+  sorted_vector_set<std::pair<int, int>> set;
+  sorted_vector_map<int, int> map;
+
+  for (size_t i = 0; i < 4; ++i) {
+    const std::pair<int, int> k00(0, 0);
+    const std::pair<int, int> k0i(0, i % 2);
+    const std::pair<int, int> k10(1, 0);
+    const std::pair<int, int> k1i(1, i % 2);
+    const std::pair<int, int> k20(2, 0);
+    const std::pair<int, int> k2i(2, i % 2);
+
+    EXPECT_EQ(*set.emplace_hint(set.begin(), 0, i % 2), k0i);
+    EXPECT_EQ(*map.emplace_hint(map.begin(), 0, i % 2), k00);
+
+    EXPECT_EQ(*set.emplace_hint(set.begin(), k1i), k1i);
+    EXPECT_EQ(*map.emplace_hint(map.begin(), k1i), k10);
+
+    EXPECT_EQ(*set.emplace_hint(set.begin(), folly::copy(k2i)), k2i);
+    EXPECT_EQ(*map.emplace_hint(map.begin(), folly::copy(k2i)), k20);
+
+    check_invariant(set);
+    check_invariant(map);
+  }
+}