Re: [PATCH][_GLIBCXX_DEBUG] Fix std::__niter_base behavior

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4647 bytes --]

On Thu, 15 Feb 2024 at 18:38, François Dumont <frs.dumont@gmail.com> wrote:

>
> On 15/02/2024 14:17, Jonathan Wakely wrote:
>
>
>
> On Wed, 14 Feb 2024 at 21:48, François Dumont <frs.dumont@gmail.com>
> wrote:
>
>>
>> On 14/02/2024 20:44, Jonathan Wakely wrote:
>>
>>
>>
>> On Wed, 14 Feb 2024 at 18:39, François Dumont <frs.dumont@gmail.com>
>> wrote:
>>
>>> libstdc++: [_GLIBCXX_DEBUG] Fix std::__niter_base behavior
>>>
>>> std::__niter_base is used in _GLIBCXX_DEBUG mode to remove
>>> _Safe_iterator<>
>>> wrapper on random access iterators. But doing so it should also preserve
>>> original
>>> behavior to remove __normal_iterator wrapper.
>>>
>>> libstdc++-v3/ChangeLog:
>>>
>>>      * include/bits/stl_algobase.h (std::__niter_base): Redefine the
>>> overload
>>>      definitions for __gnu_debug::_Safe_iterator.
>>>      * include/debug/safe_iterator.tcc (std::__niter_base): Adapt
>>> declarations.
>>>
>>> Ok to commit once all tests completed (still need to check pre-c++11) ?
>>>
>>
>>
>> The declaration in  include/bits/stl_algobase.h has a noexcept-specifier
>> but the definition in include/debug/safe_iterator.tcc does not have one -
>> that seems wrong (I'm surprised it even compiles).
>>
>> It does !
>>
>
> The diagnostic is suppressed without -Wsystem-headers:
>
> /home/jwakely/gcc/14/include/c++/14.0.1/debug/safe_iterator.tcc:255:5: warning:
> declaration of 'template<class _Ite, class _Seq> constexpr decltype
> (std::__
> niter_base(declval<_Ite>())) std::__niter_base(const
> __gnu_debug::_Safe_iterator<_Iterator, _Sequence,
> random_access_iterator_tag>&)' has a different except
> ion specifier [-Wsystem-headers]
>  255 |     __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
>      |     ^~~~~~~~~~~~
> /home/jwakely/gcc/14/include/c++/14.0.1/bits/stl_algobase.h:335:5: note: from
> previous declaration 'template<class _Ite, class _Seq> constexpr decltype
> (std
> ::__niter_base(declval<_Ite>())) std::__niter_base(const
> __gnu_debug::_Safe_iterator<_Iterator, _Sequence,
> random_access_iterator_tag>&) noexcept (noexcept
> (is_nothrow_copy_constructible<decltype
> (std::__niter_base(declval<_Ite>()))>::value))'
>  335 |     __niter_base(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq,
>      |     ^~~~~~~~~~~~
>
>
> It's a hard error with Clang though:
>
> deb.cc:7:10: error: call to '__niter_base' is ambiguous
>
>
> Yes, I eventually got the error too, I hadn't run enough tests yet.
>
>
>
>
>
>
>> I thought it was only necessary at declaration, and I also had troubles
>> doing it right at definition because of the interaction with the auto and
>> ->.
>>
>
> The trailing-return-type has to come after the noexcept-specifier.
>
>
>
>> Now simplified and consistent in this new proposal.
>>
>>
>> Just using std::is_nothrow_copy_constructible<_Ite> seems simpler, that
>> will be true for __normal_iterator<I, C> if
>> is_nothrow_copy_constructible<I> is true.
>>
>> Ok
>>
>>
>> The definition in include/debug/safe_iterator.tcc should use
>> std::declval<_Ite>() not declval<_Ite>(). Is there any reason why the
>> definition uses a late-specified-return-type (i.e. auto and ->) when the
>> declaration doesn't?
>>
>>
>> I initially plan to use '->
>> std::decltype(std::__niter_base(__it.base()))' but this did not compile,
>> ambiguity issue. So I resort to using std::declval and I could have then
>> done it the same way as declaration, done now.
>>
>> Attached is what I'm testing, ok to commit once fully tested ?
>>
>
> OK, thanks.
>
> Thanks for validation but I have a problem to test for c++98.
>
> When I do:
>
> make CXXFLAGS=-std=c++98 check-debug
>

That doesn't work any more, see
https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.run.permutations



> I see in debug/libstdc++.log for example:
>
> Executing on host: /home/fdumont/dev/gcc/build/./gcc/xg++ -shared-libgcc
> ... -mshstk -std=c++98 -g -O2 -DLOCALEDIR="." -nostdinc++
> -I/home/fdumont/dev/gcc/...
> /home/fdumont/dev/gcc/git/libstdc++-v3/testsuite/25_algorithms/copy/3.cc
> -D_GLIBCXX_DEBUG   -std=gnu++17  -include bits/stdc++.h ...  -lm  -o
> ./3.exe    (timeout = 360)
>
> The -std=c++98 is there but later comes the -std=gnu++17 so I think it
> runs in C++17, no ?
>
> I also tried the documented alternative:
>
> make check 'RUNTESTFLAGS=--target_board=unix/-O3\"{-std=gnu++98,-std=gnu++11,-std=gnu++14}\"'
>
>
> but same problem, -std=gnu++17 comes last.
>
> I'll try to rebuild all from scratch but I won't commit soon then.
>
>
>