Skip to content

[ADT] Use adl_begin/adl_end in make_filter_range #130512

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 10, 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.

Also fix potential issue with double-move on the input range.

@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 fix potential issue with double-move on the input range.


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

2 Files Affected:

  • (modified) llvm/include/llvm/ADT/STLExtras.h (+3-5)
  • (modified) llvm/unittests/ADT/IteratorTest.cpp (+25-10)
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index ace5f60b572d7..9e0b0cef9be3c 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -573,11 +573,9 @@ iterator_range<filter_iterator<detail::IterOfRange<RangeT>, PredicateT>>
 make_filter_range(RangeT &&Range, PredicateT Pred) {
   using FilterIteratorT =
       filter_iterator<detail::IterOfRange<RangeT>, PredicateT>;
-  return make_range(
-      FilterIteratorT(std::begin(std::forward<RangeT>(Range)),
-                      std::end(std::forward<RangeT>(Range)), Pred),
-      FilterIteratorT(std::end(std::forward<RangeT>(Range)),
-                      std::end(std::forward<RangeT>(Range)), Pred));
+  auto B = adl_begin(Range);
+  auto E = adl_end(Range);
+  return make_range(FilterIteratorT(B, E, Pred), FilterIteratorT(E, E, Pred));
 }
 
 /// A pseudo-iterator adaptor that is designed to implement "early increment"
diff --git a/llvm/unittests/ADT/IteratorTest.cpp b/llvm/unittests/ADT/IteratorTest.cpp
index a0d3c9b564d85..691fbce5080ff 100644
--- a/llvm/unittests/ADT/IteratorTest.cpp
+++ b/llvm/unittests/ADT/IteratorTest.cpp
@@ -22,6 +22,22 @@ using testing::ElementsAre;
 
 namespace {
 
+namespace adl_test {
+struct WithFreeBeginEnd {
+  int data[3] = {21, 22, 23};
+};
+
+auto begin(const WithFreeBeginEnd &X) { return std::begin(X.data); }
+auto end(const WithFreeBeginEnd &X) { return std::end(X.data); }
+
+struct WithFreeRBeginREnd {
+  int data[3] = {42, 43, 44};
+};
+
+auto rbegin(const WithFreeRBeginREnd &X) { return std::rbegin(X.data); }
+auto rend(const WithFreeRBeginREnd &X) { return std::rend(X.data); }
+} // namespace adl_test
+
 template <int> struct Shadow;
 
 struct WeirdIter
@@ -364,6 +380,14 @@ TEST(FilterIteratorTest, ReverseFilterRange) {
   EXPECT_EQ((SmallVector<int, 4>{6, 4, 2, 0}), Actual4);
 }
 
+TEST(FilterIteratorTest, ADL) {
+  // Make sure that we use the `begin`/`end` functions
+  // from `adl_test`, using ADL.
+  adl_test::WithFreeBeginEnd R;
+  auto IsOdd = [](int N) { return N % 2 != 0; };
+  EXPECT_THAT(make_filter_range(R, IsOdd), ElementsAre(21, 23));
+}
+
 TEST(PointerIterator, Basic) {
   int A[] = {1, 2, 3, 4};
   pointer_iterator<int *> Begin(std::begin(A)), End(std::end(A));
@@ -395,18 +419,9 @@ TEST(PointerIterator, Range) {
     EXPECT_EQ(A + I++, P);
 }
 
-namespace rbegin_detail {
-struct WithFreeRBegin {
-  int data[3] = {42, 43, 44};
-};
-
-auto rbegin(const WithFreeRBegin &X) { return std::rbegin(X.data); }
-auto rend(const WithFreeRBegin &X) { return std::rend(X.data); }
-} // namespace rbegin_detail
-
 TEST(ReverseTest, ADL) {
   // Check that we can find the rbegin/rend functions via ADL.
-  rbegin_detail::WithFreeRBegin Foo;
+  adl_test::WithFreeRBeginREnd Foo;
   EXPECT_THAT(reverse(Foo), ElementsAre(44, 43, 42));
 }
 

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 7770d8f 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