Skip to content

Conversation

kazutakahirata
Copy link
Contributor

@kazutakahirata kazutakahirata commented Aug 25, 2025

The l-value and r-value reference variants of try_emplace contain
nearly identical code. Also, operator[] makes its own call to
Map.try_emplace.

This patch introduces a templated helper function, try_emplace_impl,
and uses it in all of MapVector::insert, try_emplace, and operator[].
The helper function uses perfect forwarding to preserve the exact key
type.

This patch moves the "private:" section to the end of the class so
that the new helper function can use iterator.

The l-value and r-value reference variants of try_emplace contain
nearly identical code.  Also, operator[] makes its own call to
Map.try_emplace.

This patch introduces a templated helper function, try_emplace_impl,
and uses it in all of MapVector::insert, try_emplace, and operator[].
The helper function uses perfect forwarding to preserve the exact key
type.

This patch moves "using" declarations to the top of the class because
the new helper function needs iterator.
@llvmbot
Copy link
Member

llvmbot commented Aug 25, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

The l-value and r-value reference variants of try_emplace contain
nearly identical code. Also, operator[] makes its own call to
Map.try_emplace.

This patch introduces a templated helper function, try_emplace_impl,
and uses it in all of MapVector::insert, try_emplace, and operator[].
The helper function uses perfect forwarding to preserve the exact key
type.

This patch moves "using" declarations to the top of the class because
the new helper function needs iterator.


Full diff: https://github.com/llvm/llvm-project/pull/155205.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/MapVector.h (+27-33)
diff --git a/llvm/include/llvm/ADT/MapVector.h b/llvm/include/llvm/ADT/MapVector.h
index 24976535716d1..5f9f2b6794bc6 100644
--- a/llvm/include/llvm/ADT/MapVector.h
+++ b/llvm/include/llvm/ADT/MapVector.h
@@ -34,13 +34,6 @@ template <typename KeyT, typename ValueT,
           typename MapType = DenseMap<KeyT, unsigned>,
           typename VectorType = SmallVector<std::pair<KeyT, ValueT>, 0>>
 class MapVector {
-  MapType Map;
-  VectorType Vector;
-
-  static_assert(
-      std::is_integral_v<typename MapType::mapped_type>,
-      "The mapped_type of the specified Map must be an integral type");
-
 public:
   using key_type = KeyT;
   using value_type = typename VectorType::value_type;
@@ -51,6 +44,28 @@ class MapVector {
   using reverse_iterator = typename VectorType::reverse_iterator;
   using const_reverse_iterator = typename VectorType::const_reverse_iterator;
 
+private:
+  MapType Map;
+  VectorType Vector;
+
+  static_assert(
+      std::is_integral_v<typename MapType::mapped_type>,
+      "The mapped_type of the specified Map must be an integral type");
+
+  template <typename KeyArgT, typename... Ts>
+  std::pair<iterator, bool> try_emplace_impl(KeyArgT &&Key, Ts &&...Args) {
+    auto Result = Map.try_emplace(Key);
+    if (Result.second) {
+      Result.first->second = Vector.size();
+      Vector.emplace_back(std::piecewise_construct,
+                          std::forward_as_tuple(std::forward<KeyArgT>(Key)),
+                          std::forward_as_tuple(std::forward<Ts>(Args)...));
+      return {std::prev(end()), true};
+    }
+    return {begin() + Result.first->second, false};
+  }
+
+public:
   /// Clear the MapVector and return the underlying vector.
   VectorType takeVector() {
     Map.clear();
@@ -99,13 +114,7 @@ class MapVector {
   }
 
   ValueT &operator[](const KeyT &Key) {
-    std::pair<typename MapType::iterator, bool> Result = Map.try_emplace(Key);
-    auto &I = Result.first->second;
-    if (Result.second) {
-      Vector.push_back(std::make_pair(Key, ValueT()));
-      I = Vector.size() - 1;
-    }
-    return Vector[I].second;
+    return try_emplace_impl(Key).first->second;
   }
 
   // Returns a copy of the value.  Only allowed if ValueT is copyable.
@@ -118,33 +127,18 @@ class MapVector {
 
   template <typename... Ts>
   std::pair<iterator, bool> try_emplace(const KeyT &Key, Ts &&...Args) {
-    auto [It, Inserted] = Map.try_emplace(Key);
-    if (Inserted) {
-      It->second = Vector.size();
-      Vector.emplace_back(std::piecewise_construct, std::forward_as_tuple(Key),
-                          std::forward_as_tuple(std::forward<Ts>(Args)...));
-      return {std::prev(end()), true};
-    }
-    return {begin() + It->second, false};
+    return try_emplace_impl(Key, std::forward<Ts>(Args)...);
   }
   template <typename... Ts>
   std::pair<iterator, bool> try_emplace(KeyT &&Key, Ts &&...Args) {
-    auto [It, Inserted] = Map.try_emplace(Key);
-    if (Inserted) {
-      It->second = Vector.size();
-      Vector.emplace_back(std::piecewise_construct,
-                          std::forward_as_tuple(std::move(Key)),
-                          std::forward_as_tuple(std::forward<Ts>(Args)...));
-      return {std::prev(end()), true};
-    }
-    return {begin() + It->second, false};
+    return try_emplace_impl(std::move(Key), std::forward<Ts>(Args)...);
   }
 
   std::pair<iterator, bool> insert(const std::pair<KeyT, ValueT> &KV) {
-    return try_emplace(KV.first, KV.second);
+    return try_emplace_impl(KV.first, KV.second);
   }
   std::pair<iterator, bool> insert(std::pair<KeyT, ValueT> &&KV) {
-    return try_emplace(std::move(KV.first), std::move(KV.second));
+    return try_emplace_impl(std::move(KV.first), std::move(KV.second));
   }
 
   template <typename V>

@kazutakahirata kazutakahirata merged commit 9ae059b into llvm:main Aug 25, 2025
8 of 9 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20250824_MapVector_try_emplace_impl branch August 25, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants