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, Moritzlibstdc++-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 autobegin() requires common_range<_Vp> - { return make_reverse_iterator(ranges::end(_M_base)); } + { return std::make_reverse_iterator(ranges::end(_M_base)); }constexpr autobegin() 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 autoend() const requires common_range<const _Vp> - { return make_reverse_iterator(ranges::begin(_M_base)); } + { return std::make_reverse_iterator(ranges::begin(_M_base)); }constexpr autosize() 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
From 9f9858c24519bcea903de477a37b31eed09725c1 Mon Sep 17 00:00:00 2001
From: Moritz Sichert <sich...@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 | 40 +++++++++++++++++++
2 files changed, 46 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..7d01f5d42a2
--- /dev/null
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse_no_adl.cc
@@ -0,0 +1,40 @@
+// 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 compile { 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)>);
+ static_assert(std::ranges::view<const decltype(v)>);
+}
+
--
2.30.1
smime.p7s
Description: S/MIME Cryptographic Signature
