Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator

[Jonathan Wakely via Gcc-patches]

On 23/03/21 16:25 +0000, Jonathan Wakely wrote:

On 03/03/21 20:26 +0100, Moritz Sichert via Libstdc++ wrote:

Thanks for the review. I attached the updated patch.

Can you commit this for me or point me to what I should do next? This is my 
first contribution here.

I was about to do this, but ...

+namespace test_ns
+{
+  struct A {};
+  template <typename T>
+  void make_reverse_iterator(T&&) {}
+} // namespace test_ns
+
+void test()
+{
+  test_ns::A as[] = {{}, {}};
+  auto v = as | std::views::reverse;
+  static_assert(std::ranges::view<decltype(v)>);
+  static_assert(std::ranges::view<const decltype(v)>);

Was this tested? A view must be movable, which requires
move-assignable. You can't assign to a const view, so const
decltype(v) does not model movable so does not model view.

Here's what I've committed. Thanks for the bugfix.



commit 09f08fef71fb776a1d850a7b854c7ccf8a3d6c11
Author: Moritz Sichert <sich...@in.tum.de>
Date:   Tue Mar 23 15:47:37 2021

    libstdc++: Avoid accidental ADL when calling make_reverse_iterator
    
    std::ranges::reverse_view uses make_reverse_iterator in its
    implementation as described in [range.reverse.view]. This accidentally
    allows ADL as an unqualified name is used in the call. According to
    [contents], however, this should be treated as a qualified lookup into
    the std namespace.
    
    This leads to errors due to ambiguous name lookups when another
    make_reverse_iterator function is found via ADL.
    
    libstdc++-v3/Changelog:
    
            * include/std/ranges (reverse_view::begin, reverse_view::end):
            Qualify make_reverse_iterator calls to avoid ADL.
            * testsuite/std/ranges/adaptors/reverse.cc: Test that
            views::reverse works when make_reverse_iterator is defined
            in an associated namespace.

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 1be74beb860..adbc6d7b274 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -2958,29 +2958,29 @@ namespace views
       {
 	if constexpr (_S_needs_cached_begin)
 	  if (_M_cached_begin._M_has_value())
-	    return make_reverse_iterator(_M_cached_begin._M_get(_M_base));
+	    return std::make_reverse_iterator(_M_cached_begin._M_get(_M_base));
 
 	auto __it = ranges::next(ranges::begin(_M_base), ranges::end(_M_base));
 	if constexpr (_S_needs_cached_begin)
 	  _M_cached_begin._M_set(_M_base, __it);
-	return make_reverse_iterator(std::move(__it));
+	return std::make_reverse_iterator(std::move(__it));
       }
 
       constexpr auto
       begin() requires common_range<_Vp>
-      { return make_reverse_iterator(ranges::end(_M_base)); }
+      { return std::make_reverse_iterator(ranges::end(_M_base)); }
 
       constexpr auto
       begin() const requires common_range<const _Vp>
-      { return make_reverse_iterator(ranges::end(_M_base)); }
+      { return std::make_reverse_iterator(ranges::end(_M_base)); }
 
       constexpr reverse_iterator<iterator_t<_Vp>>
       end()
-      { return make_reverse_iterator(ranges::begin(_M_base)); }
+      { return std::make_reverse_iterator(ranges::begin(_M_base)); }
 
       constexpr auto
       end() const requires common_range<const _Vp>
-      { return make_reverse_iterator(ranges::begin(_M_base)); }
+      { return std::make_reverse_iterator(ranges::begin(_M_base)); }
 
       constexpr auto
       size() requires sized_range<_Vp>
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc
index 593b77e4a37..47386575192 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc
@@ -131,6 +131,23 @@ test05()
   VERIFY( test_wrapper<int>::increment_count == 5 );
 }
 
+namespace test_ns
+{
+  struct A {};
+  template <typename T>
+  void make_reverse_iterator(T&&) {}
+} // namespace test_ns
+
+void test06()
+{
+  // Check that views::reverse works and does not use ADL which could lead
+  // to accidentally finding test_ns::make_reverse_iterator(const A&).
+  test_ns::A as[] = {{}, {}};
+  auto v = as | std::views::reverse;
+  static_assert(std::ranges::view<decltype(v)>);
+  static_assert(std::ranges::view<decltype(v)>);
+}
+
 int
 main()
 {
@@ -139,4 +156,5 @@ main()
   test03();
   test04();
   test05();
+  test06();
 }