diff mbox series

libstdc++: Avoid accidental ADL when calling make_reverse_iterator

Message ID 0cb77c84-100b-1f4a-1570-873ef0c94f5e@in.tum.de
State New
Headers show
Series libstdc++: Avoid accidental ADL when calling make_reverse_iterator | expand

Commit Message

Moritz Sichert March 3, 2021, 5:25 p.m. UTC 
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.

I found this as llvm has its own implementation of ranges which also has llvm::make_reverse_iterator. This lead to a compile error with std::vector<llvm::Value*> | std::ranges::views::reverse.

Best,
Moritz

Comments

Patrick Palka March 3, 2021, 6:02 p.m. UTC | #1 
On Wed, 3 Mar 2021, Moritz Sichert via Libstdc++ wrote:

> 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.
> 
> I found this as llvm has its own implementation of ranges which also has
> llvm::make_reverse_iterator. This lead to a compile error with
> std::vector<llvm::Value*> | std::ranges::views::reverse.

Thanks for the patch!  It looks good to me.  Some very minor comments
below.

> 
> Best,
> Moritz
> 

> libstdc++-v3/Changelog:
> 
> 	* include/std/ranges: Avoid accidental ADL when calling
>         make_reverse_iterator.
>         * testsuite/std/ranges/adaptors/reverse_no_adl.cc: New test.
>         Test that views::reverse works when make_reverse_iterator is
>         defined in a namespace that could be found via ADL.
> ---
>  libstdc++-v3/include/std/ranges               | 12 +++---
>  .../std/ranges/adaptors/reverse_no_adl.cc     | 39 +++++++++++++++++++
>  2 files changed, 45 insertions(+), 6 deletions(-)
>  create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
> 
> 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_no_adl.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
> new file mode 100644
> index 00000000000..9aa96c7d40e
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
> @@ -0,0 +1,39 @@
> +// Copyright (C) 2020-2021 Free Software Foundation, Inc.
> +//
> +// This file is part of the GNU ISO C++ Library.  This library is free
> +// software; you can redistribute it and/or modify it under the
> +// terms of the GNU General Public License as published by the
> +// Free Software Foundation; either version 3, or (at your option)
> +// any later version.
> +
> +// This library is distributed in the hope that it will be useful,
> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +// GNU General Public License for more details.
> +
> +// You should have received a copy of the GNU General Public License along
> +// with this library; see the file COPYING3.  If not see
> +// <http://www.gnu.org/licenses/>.
> +
> +// { dg-options "-std=gnu++2a" }
> +// { dg-do run { target c++2a } }

Since this isn't an execution test, we should use "compile" instead of
"run" here.

> 
> +
> +#include <ranges>
> +
> +// This test tests that views::reverse works and does not use ADL which could
> +// lead to accidentally finding test_ns::make_reverse_iterator(const A&).
> +
> +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)>);

I suppose we could also check

  static_assert(std::ranges::range<const decltype(v)>)

which'll additionally verify the std:: qualification inside the const
begin()/end() overloads.

> +}
> +
> -- 
> 2.30.1
>
Moritz Sichert March 3, 2021, 7:26 p.m. UTC | #2 
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.

Best,
Moritz

Am 03.03.21 um 19:02 schrieb Patrick Palka:
> On Wed, 3 Mar 2021, Moritz Sichert via Libstdc++ wrote:
> 
>> 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.
>>
>> I found this as llvm has its own implementation of ranges which also has
>> llvm::make_reverse_iterator. This lead to a compile error with
>> std::vector<llvm::Value*> | std::ranges::views::reverse.
> 
> Thanks for the patch!  It looks good to me.  Some very minor comments
> below.
> 
>>
>> Best,
>> Moritz
>>
> 
>> libstdc++-v3/Changelog:
>>
>> 	* include/std/ranges: Avoid accidental ADL when calling
>>          make_reverse_iterator.
>>          * testsuite/std/ranges/adaptors/reverse_no_adl.cc: New test.
>>          Test that views::reverse works when make_reverse_iterator is
>>          defined in a namespace that could be found via ADL.
>> ---
>>   libstdc++-v3/include/std/ranges               | 12 +++---
>>   .../std/ranges/adaptors/reverse_no_adl.cc     | 39 +++++++++++++++++++
>>   2 files changed, 45 insertions(+), 6 deletions(-)
>>   create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
>>
>> 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_no_adl.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
>> new file mode 100644
>> index 00000000000..9aa96c7d40e
>> --- /dev/null
>> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
>> @@ -0,0 +1,39 @@
>> +// Copyright (C) 2020-2021 Free Software Foundation, Inc.
>> +//
>> +// This file is part of the GNU ISO C++ Library.  This library is free
>> +// software; you can redistribute it and/or modify it under the
>> +// terms of the GNU General Public License as published by the
>> +// Free Software Foundation; either version 3, or (at your option)
>> +// any later version.
>> +
>> +// This library is distributed in the hope that it will be useful,
>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +// GNU General Public License for more details.
>> +
>> +// You should have received a copy of the GNU General Public License along
>> +// with this library; see the file COPYING3.  If not see
>> +// <http://www.gnu.org/licenses/>.
>> +
>> +// { dg-options "-std=gnu++2a" }
>> +// { dg-do run { target c++2a } }
> 
> Since this isn't an execution test, we should use "compile" instead of
> "run" here.
> 
>>
>> +
>> +#include <ranges>
>> +
>> +// This test tests that views::reverse works and does not use ADL which could
>> +// lead to accidentally finding test_ns::make_reverse_iterator(const A&).
>> +
>> +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)>);
> 
> I suppose we could also check
> 
>    static_assert(std::ranges::range<const decltype(v)>)
> 
> which'll additionally verify the std:: qualification inside the const
> begin()/end() overloads.
> 
>> +}
>> +
>> -- 
>> 2.30.1
>>
>
Moritz Sichert March 19, 2021, 2:48 p.m. UTC | #3 
Quick reminder: Can you (or anyone) please commit this? The updated patch is attached in the last email.

Best,
Moritz

Am 03.03.21 um 20:26 schrieb Moritz Sichert:
> 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.
> 
> Best,
> Moritz
> 
> Am 03.03.21 um 19:02 schrieb Patrick Palka:
>> On Wed, 3 Mar 2021, Moritz Sichert via Libstdc++ wrote:
>>
>>> 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.
>>>
>>> I found this as llvm has its own implementation of ranges which also has
>>> llvm::make_reverse_iterator. This lead to a compile error with
>>> std::vector<llvm::Value*> | std::ranges::views::reverse.
>>
>> Thanks for the patch!  It looks good to me.  Some very minor comments
>> below.
>>
>>>
>>> Best,
>>> Moritz
>>>
>>
>>> libstdc++-v3/Changelog:
>>>
>>>     * include/std/ranges: Avoid accidental ADL when calling
>>>          make_reverse_iterator.
>>>          * testsuite/std/ranges/adaptors/reverse_no_adl.cc: New test.
>>>          Test that views::reverse works when make_reverse_iterator is
>>>          defined in a namespace that could be found via ADL.
>>> ---
>>>   libstdc++-v3/include/std/ranges               | 12 +++---
>>>   .../std/ranges/adaptors/reverse_no_adl.cc     | 39 +++++++++++++++++++
>>>   2 files changed, 45 insertions(+), 6 deletions(-)
>>>   create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
>>>
>>> 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_no_adl.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
>>> new file mode 100644
>>> index 00000000000..9aa96c7d40e
>>> --- /dev/null
>>> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
>>> @@ -0,0 +1,39 @@
>>> +// Copyright (C) 2020-2021 Free Software Foundation, Inc.
>>> +//
>>> +// This file is part of the GNU ISO C++ Library.  This library is free
>>> +// software; you can redistribute it and/or modify it under the
>>> +// terms of the GNU General Public License as published by the
>>> +// Free Software Foundation; either version 3, or (at your option)
>>> +// any later version.
>>> +
>>> +// This library is distributed in the hope that it will be useful,
>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +// GNU General Public License for more details.
>>> +
>>> +// You should have received a copy of the GNU General Public License along
>>> +// with this library; see the file COPYING3.  If not see
>>> +// <http://www.gnu.org/licenses/>.
>>> +
>>> +// { dg-options "-std=gnu++2a" }
>>> +// { dg-do run { target c++2a } }
>>
>> Since this isn't an execution test, we should use "compile" instead of
>> "run" here.
>>
>>>
>>> +
>>> +#include <ranges>
>>> +
>>> +// This test tests that views::reverse works and does not use ADL which could
>>> +// lead to accidentally finding test_ns::make_reverse_iterator(const A&).
>>> +
>>> +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)>);
>>
>> I suppose we could also check
>>
>>    static_assert(std::ranges::range<const decltype(v)>)
>>
>> which'll additionally verify the std:: qualification inside the const
>> begin()/end() overloads.
>>
>>> +}
>>> +
>>> -- 
>>> 2.30.1
>>>
>>
Jonathan Wakely March 23, 2021, 4:25 p.m. UTC | #4 
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.
Jonathan Wakely March 23, 2021, 5:09 p.m. UTC | #5 
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.
Moritz Sichert March 23, 2021, 5:45 p.m. UTC | #6 
Thank you!

You are right. The idea was that the test also tests the const overloads of begin() and end() of reverse_view. But the view concept requires movable. Maybe, this should just be

static_assert(std::ranges::range<const decltype(v)>);

instead?

The test in the patch now has the same static_assert twice.

Am 23.03.21 um 18:09 schrieb Jonathan Wakely:
> 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.
> 
>
Jonathan Wakely March 23, 2021, 6:25 p.m. UTC | #7 
On 23/03/21 18:45 +0100, Moritz Sichert via Libstdc++ wrote:
>Thank you!
>
>You are right. The idea was that the test also tests the const overloads of begin() and end() of reverse_view. But the view concept requires movable. Maybe, this should just be
>
>static_assert(std::ranges::range<const decltype(v)>);
>
>instead?

Oops, I mean to delete the second line, not just the 'const' keyword.
But checking that it models range is better.

Also the comment is slightly wrong, it won't call
make_reserve_iterator(const A&) it will call make_reserve_iterator(A*)
or make_reserve_iterator(const A*).

I've committed the attached patch. Thanks again.
diff mbox series

Patch

From f9aaf991c75047f83f12a98311cb62278c32dcda Mon Sep 17 00:00:00 2001
From: Moritz Sichert <sichert@in.tum.de>
Date: Wed, 3 Mar 2021 18:14:28 +0100
Subject: [PATCH] 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: Avoid accidental ADL when calling
        make_reverse_iterator.
        * testsuite/std/ranges/adaptors/reverse_no_adl.cc: New test.
        Test that views::reverse works when make_reverse_iterator is
        defined in a namespace that could be found via ADL.
---
 libstdc++-v3/include/std/ranges               | 12 +++---
 .../std/ranges/adaptors/reverse_no_adl.cc     | 39 +++++++++++++++++++
 2 files changed, 45 insertions(+), 6 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc

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_no_adl.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
new file mode 100644
index 00000000000..9aa96c7d40e
--- /dev/null
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
@@ -0,0 +1,39 @@ 
+// Copyright (C) 2020-2021 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do run { target c++2a } }
+
+#include <ranges>
+
+// This test tests that views::reverse works and does not use ADL which could
+// lead to accidentally finding test_ns::make_reverse_iterator(const A&).
+
+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)>);
+}
+
-- 
2.30.1