Skip to content

[ADT] Use adl_begin in make_first_range and make_second_range #130521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 12, 2025

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.

This PR is stacked on top of #130508.

@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.

This PR is stacked on top of #130508.


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

2 Files Affected:

  • (modified) llvm/include/llvm/ADT/STLExtras.h (+3-4)
  • (modified) llvm/unittests/ADT/STLExtrasTest.cpp (+30)
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index fe1d010574e31..8600c59d9306d 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -376,8 +376,7 @@ inline mapped_iterator<ItTy, FuncTy> map_iterator(ItTy I, FuncTy F) {
 
 template <class ContainerTy, class FuncTy>
 auto map_range(ContainerTy &&C, FuncTy F) {
-  return make_range(map_iterator(std::begin(C), F),
-                    map_iterator(std::end(C), F));
+  return make_range(map_iterator(adl_begin(C), F), map_iterator(adl_end(C), F));
 }
 
 /// A base type of mapped iterator, that is useful for building derived
@@ -1438,7 +1437,7 @@ template <typename EltTy, typename FirstTy> class first_or_second_type {
 
 /// Given a container of pairs, return a range over the first elements.
 template <typename ContainerTy> auto make_first_range(ContainerTy &&c) {
-  using EltTy = decltype((*std::begin(c)));
+  using EltTy = decltype(*adl_begin(c));
   return llvm::map_range(std::forward<ContainerTy>(c),
                          [](EltTy elt) -> typename detail::first_or_second_type<
                                            EltTy, decltype((elt.first))>::type {
@@ -1448,7 +1447,7 @@ template <typename ContainerTy> auto make_first_range(ContainerTy &&c) {
 
 /// Given a container of pairs, return a range over the second elements.
 template <typename ContainerTy> auto make_second_range(ContainerTy &&c) {
-  using EltTy = decltype((*std::begin(c)));
+  using EltTy = decltype(*adl_begin(c));
   return llvm::map_range(
       std::forward<ContainerTy>(c),
       [](EltTy elt) ->
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index e107cb570641b..6d1478a9102a6 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -421,6 +421,15 @@ void swap(some_struct &lhs, some_struct &rhs) {
   rhs.swap_val = "rhs";
 }
 
+struct Pairs {
+  std::vector<std::pair<std::string, int>> data;
+  using const_iterator =
+      std::vector<std::pair<std::string, int>>::const_iterator;
+};
+
+Pairs::const_iterator begin(const Pairs &p) { return p.data.begin(); }
+Pairs::const_iterator end(const Pairs &p) { return p.data.end(); }
+
 struct requires_move {};
 int *begin(requires_move &&) { return nullptr; }
 int *end(requires_move &&) { return nullptr; }
@@ -504,6 +513,15 @@ TEST(STLExtrasTest, ConcatRange) {
   EXPECT_EQ(Expected, Test);
 }
 
+TEST(STLExtrasTest, MakeFirstSecondRangeADL) {
+  // Make sure that we use the `begin`/`end` functions from `some_namespace`,
+  // using ADL.
+  some_namespace::Pairs Pairs;
+  Pairs.data = {{"foo", 1}, {"bar", 2}};
+  EXPECT_THAT(make_first_range(Pairs), ElementsAre("foo", "bar"));
+  EXPECT_THAT(make_second_range(Pairs), ElementsAre(1, 2));
+}
+
 template <typename T> struct Iterator {
   int i = 0;
   T operator*() const { return i; }
@@ -730,6 +748,18 @@ TEST(STLExtrasTest, DropEndDefaultTest) {
   EXPECT_EQ(i, 4);
 }
 
+TEST(STLExtrasTest, MapRangeTest) {
+  SmallVector<int, 5> Vec{0, 1, 2};
+  EXPECT_THAT(map_range(Vec, [](int V) { return V + 1; }),
+              ElementsAre(1, 2, 3));
+
+  // Make sure that we use the `begin`/`end` functions
+  // from `some_namespace`, using ADL.
+  some_namespace::some_struct S;
+  S.data = {3, 4, 5};
+  EXPECT_THAT(map_range(S, [](int V) { return V * 2; }), ElementsAre(6, 8, 10));
+}
+
 TEST(STLExtrasTest, EarlyIncrementTest) {
   std::list<int> L = {1, 2, 3, 4};
 

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.

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.

This PR is stacked on top of llvm#130508.
@kuhar kuhar force-pushed the adl-first-second-range branch from 0139378 to 1904e2e Compare March 12, 2025 02:42
@kuhar kuhar merged commit e427f06 into llvm:main Mar 12, 2025
6 of 10 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