* [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
@ 2021-03-03 17:25 Moritz Sichert
2021-03-03 18:02 ` Patrick Palka
0 siblings, 1 reply; 8+ messages in thread
From: Moritz Sichert @ 2021-03-03 17:25 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 611 bytes --]
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
[-- Attachment #2: libstdc-Avoid-accidental-ADL-when-calling-make_rever.patch --]
[-- Type: text/x-patch, Size: 4545 bytes --]
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-03 17:25 [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator Moritz Sichert
@ 2021-03-03 18:02 ` Patrick Palka
2021-03-03 19:26 ` Moritz Sichert
0 siblings, 1 reply; 8+ messages in thread
From: Patrick Palka @ 2021-03-03 18:02 UTC (permalink / raw)
To: Moritz Sichert; +Cc: libstdc++, gcc-patches
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-03 18:02 ` Patrick Palka
@ 2021-03-03 19:26 ` Moritz Sichert
2021-03-19 14:48 ` Moritz Sichert
2021-03-23 16:25 ` Jonathan Wakely
0 siblings, 2 replies; 8+ messages in thread
From: Moritz Sichert @ 2021-03-03 19:26 UTC (permalink / raw)
To: Patrick Palka; +Cc: libstdc++, gcc-patches
[-- Attachment #1.1: Type: text/plain, Size: 5588 bytes --]
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
>>
>
[-- Attachment #1.2: libstdc-Avoid-accidental-ADL-when-calling-make_rever.patch --]
[-- Type: text/x-patch, Size: 4606 bytes --]
From 9f9858c24519bcea903de477a37b31eed09725c1 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 | 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
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5622 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-03 19:26 ` Moritz Sichert
@ 2021-03-19 14:48 ` Moritz Sichert
2021-03-23 16:25 ` Jonathan Wakely
1 sibling, 0 replies; 8+ messages in thread
From: Moritz Sichert @ 2021-03-19 14:48 UTC (permalink / raw)
To: Patrick Palka; +Cc: libstdc++, gcc-patches, Jonathan Wakely
[-- Attachment #1: Type: text/plain, Size: 6130 bytes --]
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
>>>
>>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5622 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-03 19:26 ` Moritz Sichert
2021-03-19 14:48 ` Moritz Sichert
@ 2021-03-23 16:25 ` Jonathan Wakely
2021-03-23 17:09 ` Jonathan Wakely
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2021-03-23 16:25 UTC (permalink / raw)
To: Moritz Sichert; +Cc: Patrick Palka, libstdc++, gcc-patches
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-23 16:25 ` Jonathan Wakely
@ 2021-03-23 17:09 ` Jonathan Wakely
2021-03-23 17:45 ` Moritz Sichert
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2021-03-23 17:09 UTC (permalink / raw)
To: Moritz Sichert; +Cc: Patrick Palka, libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 886 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 3540 bytes --]
commit 09f08fef71fb776a1d850a7b854c7ccf8a3d6c11
Author: Moritz Sichert <sichert@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();
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-23 17:09 ` Jonathan Wakely
@ 2021-03-23 17:45 ` Moritz Sichert
2021-03-23 18:25 ` Jonathan Wakely
0 siblings, 1 reply; 8+ messages in thread
From: Moritz Sichert @ 2021-03-23 17:45 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Patrick Palka, libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]
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.
>
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5622 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator
2021-03-23 17:45 ` Moritz Sichert
@ 2021-03-23 18:25 ` Jonathan Wakely
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Wakely @ 2021-03-23 18:25 UTC (permalink / raw)
To: Moritz Sichert; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 648 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 1208 bytes --]
commit d1aa5f57db7c14a62e9b7e2a2aa8a9c402a89363
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Tue Mar 23 18:22:18 2021
libstdc++: Improve test for views::reverse
libstdc++-v3/ChangeLog:
* testsuite/std/ranges/adaptors/reverse.cc: Replace duplicated
line with a check that uses the const being/end overloads.
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc
index 47386575192..0d52498e207 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/reverse.cc
@@ -141,11 +141,12 @@ 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&).
+ // to accidentally finding test_ns::make_reverse_iterator(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)>);
+ using V = decltype(v);
+ static_assert( std::ranges::view<V> );
+ static_assert( std::ranges::range<const V> );
}
int
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-23 18:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 17:25 [PATCH] libstdc++: Avoid accidental ADL when calling make_reverse_iterator Moritz Sichert
2021-03-03 18:02 ` Patrick Palka
2021-03-03 19:26 ` Moritz Sichert
2021-03-19 14:48 ` Moritz Sichert
2021-03-23 16:25 ` Jonathan Wakely
2021-03-23 17:09 ` Jonathan Wakely
2021-03-23 17:45 ` Moritz Sichert
2021-03-23 18:25 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).