[ADT] Allow reverse to find free rbegin/rend functions

[kuhar]

Lift the requirement that rbegin/rend must be member functions. Also allow the rbegin/rend to be found through Argument Dependent Lookup (ADL) and add adl_rbegin/adl_rend to STLExtras.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Jakub Kuderski (kuhar)

Changes

Lift the requirement that rbegin/rend must be member functions. Also allow the rbegin/rend to be found through Argument Dependent Lookup (ADL) and add adl_rbegin/adl_rend to STLExtras.

---

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

5 Files Affected:

• (modified) llvm/include/llvm/ADT/ADL.h (+32)
• (modified) llvm/include/llvm/ADT/STLExtras.h (+12-21)
• (modified) llvm/unittests/ADT/IteratorTest.cpp (+15)
• (modified) llvm/unittests/ADT/RangeAdapterTest.cpp (-4)
• (modified) llvm/unittests/ADT/STLExtrasTest.cpp (+10)

diff --git a/llvm/include/llvm/ADT/ADL.h b/llvm/include/llvm/ADT/ADL.h
index ab1f28ff6b9c09..812d9a4b52d818 100644
--- a/llvm/include/llvm/ADT/ADL.h
+++ b/llvm/include/llvm/ADT/ADL.h
@@ -37,6 +37,22 @@ constexpr auto end_impl(RangeT &&range)
   return end(std::forward<RangeT>(range));
 }
 
+using std::rbegin;
+
+template <typename RangeT>
+constexpr auto rbegin_impl(RangeT &&range)
+    -> decltype(rbegin(std::forward<RangeT>(range))) {
+  return rbegin(std::forward<RangeT>(range));
+}
+
+using std::rend;
+
+template <typename RangeT>
+constexpr auto rend_impl(RangeT &&range)
+    -> decltype(rend(std::forward<RangeT>(range))) {
+  return rend(std::forward<RangeT>(range));
+}
+
 using std::swap;
 
 template <typename T>
@@ -72,6 +88,22 @@ constexpr auto adl_end(RangeT &&range)
   return adl_detail::end_impl(std::forward<RangeT>(range));
 }
 
+/// Returns the reverse-begin iterator to \p range using `std::rbegin` and
+/// function found through Argument-Dependent Lookup (ADL).
+template <typename RangeT>
+constexpr auto adl_rbegin(RangeT &&range)
+    -> decltype(adl_detail::rbegin_impl(std::forward<RangeT>(range))) {
+  return adl_detail::rbegin_impl(std::forward<RangeT>(range));
+}
+
+/// Returns the reverse-end iterator to \p range using `std::rend` and
+/// functions found through Argument-Dependent Lookup (ADL).
+template <typename RangeT>
+constexpr auto adl_rend(RangeT &&range)
+    -> decltype(adl_detail::rend_impl(std::forward<RangeT>(range))) {
+  return adl_detail::rend_impl(std::forward<RangeT>(range));
+}
+
 /// Swaps \p lhs with \p rhs using `std::swap` and functions found through
 /// Argument-Dependent Lookup (ADL).
 template <typename T>
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index b5eb319ccf346e..08a708e5c5871e 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -405,32 +405,23 @@ class mapped_iterator_base
   }
 };
 
-/// Helper to determine if type T has a member called rbegin().
-template <typename Ty> class has_rbegin_impl {
-  using yes = char[1];
-  using no = char[2];
-
-  template <typename Inner>
-  static yes& test(Inner *I, decltype(I->rbegin()) * = nullptr);
-
-  template <typename>
-  static no& test(...);
-
-public:
-  static const bool value = sizeof(test<Ty>(nullptr)) == sizeof(yes);
-};
+namespace detail {
+template <typename Range>
+using check_has_free_function_rbegin =
+    decltype(adl_rbegin(std::declval<Range &>()));
 
-/// Metafunction to determine if T& or T has a member called rbegin().
-template <typename Ty>
-struct has_rbegin : has_rbegin_impl<std::remove_reference_t<Ty>> {};
+template <typename Range>
+static constexpr bool HasFreeFunctionRBegin =
+    is_detected<check_has_free_function_rbegin, Range>::value;
+} // namespace detail
 
 // Returns an iterator_range over the given container which iterates in reverse.
 template <typename ContainerTy> auto reverse(ContainerTy &&C) {
-  if constexpr (has_rbegin<ContainerTy>::value)
-    return make_range(C.rbegin(), C.rend());
+  if constexpr (detail::HasFreeFunctionRBegin<ContainerTy>)
+    return make_range(adl_rbegin(C), adl_rend(C));
   else
-    return make_range(std::make_reverse_iterator(std::end(C)),
-                      std::make_reverse_iterator(std::begin(C)));
+    return make_range(std::make_reverse_iterator(adl_end(C)),
+                      std::make_reverse_iterator(adl_begin(C)));
 }
 
 /// An iterator adaptor that filters the elements of given inner iterators.
diff --git a/llvm/unittests/ADT/IteratorTest.cpp b/llvm/unittests/ADT/IteratorTest.cpp
index 3a4a5b02b1040f..a0d3c9b564d857 100644
--- a/llvm/unittests/ADT/IteratorTest.cpp
+++ b/llvm/unittests/ADT/IteratorTest.cpp
@@ -395,6 +395,21 @@ 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;
+  EXPECT_THAT(reverse(Foo), ElementsAre(44, 43, 42));
+}
+
 TEST(ZipIteratorTest, Basic) {
   using namespace std;
   const SmallVector<unsigned, 6> pi{3, 1, 4, 1, 5, 9};
diff --git a/llvm/unittests/ADT/RangeAdapterTest.cpp b/llvm/unittests/ADT/RangeAdapterTest.cpp
index eb75ac301805bc..c1a8a984f233b0 100644
--- a/llvm/unittests/ADT/RangeAdapterTest.cpp
+++ b/llvm/unittests/ADT/RangeAdapterTest.cpp
@@ -150,10 +150,6 @@ TYPED_TEST(RangeAdapterRValueTest, TrivialOperation) {
   TestRev(reverse(TypeParam({0, 1, 2, 3})));
 }
 
-TYPED_TEST(RangeAdapterRValueTest, HasRbegin) {
-  static_assert(has_rbegin<TypeParam>::value, "rbegin() should be defined");
-}
-
 TYPED_TEST(RangeAdapterRValueTest, RangeType) {
   static_assert(
       std::is_same_v<decltype(reverse(std::declval<TypeParam>()).begin()),
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index b73891b59f0264..3927bc59c031a3 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -405,6 +405,14 @@ std::vector<int>::const_iterator end(const some_struct &s) {
   return s.data.end();
 }
 
+std::vector<int>::const_reverse_iterator rbegin(const some_struct &s) {
+  return s.data.rbegin();
+}
+
+std::vector<int>::const_reverse_iterator rend(const some_struct &s) {
+  return s.data.rend();
+}
+
 void swap(some_struct &lhs, some_struct &rhs) {
   // make swap visible as non-adl swap would even seem to
   // work with std::swap which defaults to moving
@@ -573,6 +581,8 @@ TEST(STLExtrasTest, ADLTest) {
 
   EXPECT_EQ(*adl_begin(s), 1);
   EXPECT_EQ(*(adl_end(s) - 1), 5);
+  EXPECT_EQ(*adl_rbegin(s), 5);
+  EXPECT_EQ(*(adl_rend(s) - 1), 1);
 
   adl_swap(s, s2);
   EXPECT_EQ(s.swap_val, "lhs");

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 5748ad84e5e8e5621f221199cc290666f00e2a30 40c47d8a3126a64b2b89ea8f909cffb38531a681 -- llvm/include/llvm/ADT/ADL.h llvm/include/llvm/ADT/STLExtras.h llvm/unittests/ADT/IteratorTest.cpp llvm/unittests/ADT/RangeAdapterTest.cpp llvm/unittests/ADT/STLExtrasTest.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/ADT/ADL.h b/llvm/include/llvm/ADT/ADL.h
index 812d9a4b52..b4a79e178c 100644
--- a/llvm/include/llvm/ADT/ADL.h
+++ b/llvm/include/llvm/ADT/ADL.h
@@ -40,16 +40,16 @@ constexpr auto end_impl(RangeT &&range)
 using std::rbegin;
 
 template <typename RangeT>
-constexpr auto rbegin_impl(RangeT &&range)
-    -> decltype(rbegin(std::forward<RangeT>(range))) {
+constexpr auto
+rbegin_impl(RangeT &&range) -> decltype(rbegin(std::forward<RangeT>(range))) {
   return rbegin(std::forward<RangeT>(range));
 }
 
 using std::rend;
 
 template <typename RangeT>
-constexpr auto rend_impl(RangeT &&range)
-    -> decltype(rend(std::forward<RangeT>(range))) {
+constexpr auto
+rend_impl(RangeT &&range) -> decltype(rend(std::forward<RangeT>(range))) {
   return rend(std::forward<RangeT>(range));
 }
 

[jroelofs]

LGTM

[kuhar]

My version of clang format (16) thinks the code is already formatted, and in addition it is consistent with the other adl functions above, so I'm going to leave this as-is without appeasing the formatting bot.