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

[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
>