Skip to content

Conversation

@kuhar
Copy link
Member

@kuhar kuhar commented Mar 9, 2025

This is to make sure that ADT helpers consistently use argument dependent lookup when dealing with input ranges.

This was a part of #87936 but reverted due to buildbot failures.

Also clean up the implementation to adhere to the llvm coding standards.

This is to make sure that ADT helpers consistently use argument dependent
lookup when dealing with input ranges.

This was a part of llvm#87936 but reverted due to buildbot failures.

Also clean up the implementation to adhere to the llvm coding standards.
@llvmbot
Copy link
Member

llvmbot commented Mar 9, 2025

@llvm/pr-subscribers-llvm-adt

Author: Jakub Kuderski (kuhar)

Changes

This is to make sure that ADT helpers consistently use argument dependent lookup when dealing with input ranges.

This was a part of #87936 but reverted due to buildbot failures.

Also clean up the implementation to adhere to the llvm coding standards.


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

2 Files Affected:

  • (modified) llvm/include/llvm/ADT/STLExtras.h (+10-7)
  • (modified) llvm/unittests/ADT/STLExtrasTest.cpp (+10)
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index fe1d010574e31..4219c6b41eac4 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -2127,7 +2127,7 @@ void append_values(Container &C, Args &&...Values) {
 
 /// Given a sequence container Cont, replace the range [ContIt, ContEnd) with
 /// the range [ValIt, ValEnd) (which is not from the same container).
-template<typename Container, typename RandomAccessIterator>
+template <typename Container, typename RandomAccessIterator>
 void replace(Container &Cont, typename Container::iterator ContIt,
              typename Container::iterator ContEnd, RandomAccessIterator ValIt,
              RandomAccessIterator ValEnd) {
@@ -2135,21 +2135,24 @@ void replace(Container &Cont, typename Container::iterator ContIt,
     if (ValIt == ValEnd) {
       Cont.erase(ContIt, ContEnd);
       return;
-    } else if (ContIt == ContEnd) {
+    }
+    if (ContIt == ContEnd) {
       Cont.insert(ContIt, ValIt, ValEnd);
       return;
     }
-    *ContIt++ = *ValIt++;
+    *ContIt = *ValIt;
+    ++ContIt;
+    ++ValIt;
   }
 }
 
 /// Given a sequence container Cont, replace the range [ContIt, ContEnd) with
 /// the range R.
-template<typename Container, typename Range = std::initializer_list<
-                                 typename Container::value_type>>
+template <typename Container, typename Range = std::initializer_list<
+                                  typename Container::value_type>>
 void replace(Container &Cont, typename Container::iterator ContIt,
-             typename Container::iterator ContEnd, Range R) {
-  replace(Cont, ContIt, ContEnd, R.begin(), R.end());
+             typename Container::iterator ContEnd, Range &&R) {
+  replace(Cont, ContIt, ContEnd, adl_begin(R), adl_end(R));
 }
 
 /// An STL-style algorithm similar to std::for_each that applies a second
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index e107cb570641b..3a34b6673ef2d 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -854,6 +854,16 @@ TEST(STLExtrasTest, EarlyIncrementTestCustomPointerIterator) {
   EXPECT_EQ(EIR.end(), I);
 }
 
+TEST(STLExtrasTest, ReplaceADL) {
+  // Make sure that we use the `begin`/`end` functions from `some_namespace`,
+  // using ADL.
+  std::vector<int> Cont = {0, 1, 2, 3, 4, 5};
+  some_namespace::some_struct S;
+  S.data = {42, 43, 44};
+  replace(Cont, Cont.begin() + 2, Cont.begin() + 5, S);
+  EXPECT_THAT(Cont, ElementsAre(0, 1, 42, 43, 44, 5));
+}
+
 TEST(STLExtrasTest, AllEqual) {
   std::vector<int> V;
   EXPECT_TRUE(all_equal(V));

Copy link
Contributor

@kazutakahirata kazutakahirata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kuhar kuhar merged commit c88352d into llvm:main Mar 10, 2025
13 checks passed
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.

3 participants