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

[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
>>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libstdc-Avoid-accidental-ADL-when-calling-make_rever.patch
Type: text/x-patch
Size: 4489 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20210303/0e0b60ca/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5622 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20210303/0e0b60ca/attachment-0001.p7s>