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 >