[PATCH] __debug::list use C++11 direct initialization

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] __debug::list use C++11 direct initialization

François Dumont-2
Here is the communication for my yesterday's patch which I thought svn
had failed to commit (I had to interrupt it).

Similarly to what I've done for associative containers here is a cleanup
of the std::__debug::list implementation leveraging more on C++11 direct
initialization.

I also made sure we use consistent comparison between
iterator/const_iterator in erase and emplace methods.

2018-10-08  François Dumont <[hidden email]>

     * include/debug/list (list<>::cbegin()): Use C++11 direct
     initialization.
     (list<>::cend()): Likewise.
     (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
     (list<>::insert(const_iterator, initializer_list<>)): Likewise.
     (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
     (list<>::erase(const_iterator, const_iterator)): Ensure consistent
     iterator comparisons.
     (list<>::splice(const_iterator, list&&, const_iterator,
     const_iterator)): Likewise.

Tested under Linux x86_64 Debug mode and committed.

François


debug_list.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] __debug::list use C++11 direct initialization

François Dumont-2
On 10/16/2018 10:15 AM, Jonathan Wakely wrote:

> On 16/10/18 07:06 +0200, François Dumont wrote:
>> On 10/15/2018 12:07 PM, Jonathan Wakely wrote:
>>> On 09/10/18 07:11 +0200, François Dumont wrote:
>>>> Here is the communication for my yesterday's patch which I thought
>>>> svn had failed to commit (I had to interrupt it).
>>>>
>>>> Similarly to what I've done for associative containers here is a
>>>> cleanup of the std::__debug::list implementation leveraging more on
>>>> C++11 direct initialization.
>>>>
>>>> I also made sure we use consistent comparison between
>>>> iterator/const_iterator in erase and emplace methods.
>>>>
>>>> 2018-10-08  François Dumont <[hidden email]>
>>>>
>>>>     * include/debug/list (list<>::cbegin()): Use C++11 direct
>>>>     initialization.
>>>>     (list<>::cend()): Likewise.
>>>>     (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
>>>>     (list<>::insert(const_iterator, initializer_list<>)): Likewise.
>>>>     (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
>>>>     (list<>::erase(const_iterator, const_iterator)): Ensure consistent
>>>>     iterator comparisons.
>>>>     (list<>::splice(const_iterator, list&&, const_iterator,
>>>>     const_iterator)): Likewise.
>>>>
>>>> Tested under Linux x86_64 Debug mode and committed.
>>>>
>>>> François
>>>>
>>>
>>>> diff --git a/libstdc++-v3/include/debug/list
>>>> b/libstdc++-v3/include/debug/list
>>>> index 8add1d596e0..879e1177497 100644
>>>> --- a/libstdc++-v3/include/debug/list
>>>> +++ b/libstdc++-v3/include/debug/list
>>>> @@ -244,11 +244,11 @@ namespace __debug
>>>> #if __cplusplus >= 201103L
>>>>       const_iterator
>>>>       cbegin() const noexcept
>>>> -      { return const_iterator(_Base::begin(), this); }
>>>> +      { return { _Base::begin(), this }; }
>>>>
>>>>       const_iterator
>>>>       cend() const noexcept
>>>> -      { return const_iterator(_Base::end(), this); }
>>>> +      { return { _Base::end(), this }; }
>>>
>>> For functions like emplace (which are C++11-only) and for forward_list
>>> (also C++11-only) using this syntax makes it clearer.
>>>
>>> But for these functions it just makes cbegin() and cend() look
>>> different to the C++98 begin() and end() functions, for no obvious
>>> benefit.
>>>
>>> Simply using { return end(); } would have been another option.
>>>
>> Personnaly I hesitated in writting:
>>
>> { return { _Base::cbegin(), this }; }
>>
>> cause I prefer when you see clearly that Debug implem forward calls
>> to the Normal implem.
>>
>> I thought that using C++11 direct init was more likely to have gcc
>> elide the copy constructor so I used it everywhere possible.
>
> It should make no difference to elision at all.
>
>
Do you want me to restore constructor call ?

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] __debug::list use C++11 direct initialization

Jonathan Wakely-3
In reply to this post by François Dumont-2
On 09/10/18 07:11 +0200, François Dumont wrote:

>Here is the communication for my yesterday's patch which I thought svn
>had failed to commit (I had to interrupt it).
>
>Similarly to what I've done for associative containers here is a
>cleanup of the std::__debug::list implementation leveraging more on
>C++11 direct initialization.
>
>I also made sure we use consistent comparison between
>iterator/const_iterator in erase and emplace methods.
>
>2018-10-08  François Dumont <[hidden email]>
>
>    * include/debug/list (list<>::cbegin()): Use C++11 direct
>    initialization.
>    (list<>::cend()): Likewise.
>    (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
>    (list<>::insert(const_iterator, initializer_list<>)): Likewise.
>    (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
>    (list<>::erase(const_iterator, const_iterator)): Ensure consistent
>    iterator comparisons.
>    (list<>::splice(const_iterator, list&&, const_iterator,
>    const_iterator)): Likewise.
>
>Tested under Linux x86_64 Debug mode and committed.
>
>François
>

>diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
>index 8add1d596e0..879e1177497 100644
>--- a/libstdc++-v3/include/debug/list
>+++ b/libstdc++-v3/include/debug/list
>@@ -244,11 +244,11 @@ namespace __debug
> #if __cplusplus >= 201103L
>       const_iterator
>       cbegin() const noexcept
>-      { return const_iterator(_Base::begin(), this); }
>+      { return { _Base::begin(), this }; }
>
>       const_iterator
>       cend() const noexcept
>-      { return const_iterator(_Base::end(), this); }
>+      { return { _Base::end(), this }; }
>
>       const_reverse_iterator
>       crbegin() const noexcept
>@@ -405,8 +405,8 @@ namespace __debug
> emplace(const_iterator __position, _Args&&... __args)
> {
>  __glibcxx_check_insert(__position);
>-  return iterator(_Base::emplace(__position.base(),
>- std::forward<_Args>(__args)...), this);
>+  return  { _Base::emplace(__position.base(),
>+   std::forward<_Args>(__args)...), this };
> }
> #endif
>
>@@ -430,7 +430,7 @@ namespace __debug
>       insert(const_iterator __p, initializer_list<value_type> __l)
>       {
> __glibcxx_check_insert(__p);
>- return iterator(_Base::insert(__p.base(), __l), this);
>+ return { _Base::insert(__p.base(), __l), this };
>       }
> #endif
>
>@@ -439,7 +439,7 @@ namespace __debug
>       insert(const_iterator __position, size_type __n, const _Tp& __x)
>       {
> __glibcxx_check_insert(__position);
>- return iterator(_Base::insert(__position.base(), __n, __x), this);
>+ return { _Base::insert(__position.base(), __n, __x), this };
>       }
> #else
>       void
>@@ -465,7 +465,7 @@ namespace __debug
> _Base::insert(__position.base(),
>      __gnu_debug::__unsafe(__first),
>      __gnu_debug::__unsafe(__last)),
>-  this
>+ this
>      };
>  else
>    return { _Base::insert(__position.base(), __first, __last), this };
>@@ -521,15 +521,17 @@ namespace __debug
> // _GLIBCXX_RESOLVE_LIB_DEFECTS
> // 151. can't currently clear() empty container
> __glibcxx_check_erase_range(__first, __last);
>- for (_Base_const_iterator __victim = __first.base();
>+ for (__decltype(__first.base()) __victim = __first.base();

This change causes the regression.

I think the problem is that the decltype is a reference, so every time
the loop does ++__victim it changes the iterator stored in __first.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] __debug::list use C++11 direct initialization

Jonathan Wakely-3
On 17/10/18 17:03 +0100, Jonathan Wakely wrote:

>On 09/10/18 07:11 +0200, François Dumont wrote:
>>Here is the communication for my yesterday's patch which I thought
>>svn had failed to commit (I had to interrupt it).
>>
>>Similarly to what I've done for associative containers here is a
>>cleanup of the std::__debug::list implementation leveraging more on
>>C++11 direct initialization.
>>
>>I also made sure we use consistent comparison between
>>iterator/const_iterator in erase and emplace methods.
>>
>>2018-10-08  François Dumont <[hidden email]>
>>
>>    * include/debug/list (list<>::cbegin()): Use C++11 direct
>>    initialization.
>>    (list<>::cend()): Likewise.
>>    (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
>>    (list<>::insert(const_iterator, initializer_list<>)): Likewise.
>>    (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
>>    (list<>::erase(const_iterator, const_iterator)): Ensure consistent
>>    iterator comparisons.
>>    (list<>::splice(const_iterator, list&&, const_iterator,
>>    const_iterator)): Likewise.
>>
>>Tested under Linux x86_64 Debug mode and committed.
>>
>>François
>>
>
>>diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
>>index 8add1d596e0..879e1177497 100644
>>--- a/libstdc++-v3/include/debug/list
>>+++ b/libstdc++-v3/include/debug/list
>>@@ -521,15 +521,17 @@ namespace __debug
>> // _GLIBCXX_RESOLVE_LIB_DEFECTS
>> // 151. can't currently clear() empty container
>> __glibcxx_check_erase_range(__first, __last);
>>- for (_Base_const_iterator __victim = __first.base();
>>+ for (__decltype(__first.base()) __victim = __first.base();
>
>This change causes the regression.
>
>I think the problem is that the decltype is a reference, so every time
>the loop does ++__victim it changes the iterator stored in __first.


This would work:

        typedef typename __decltype(__first)::iterator_type _Base_iter;
        for (_Base_iter __victim = __first.base();
             __victim != __last.base(); ++__victim)


Or simply:

#if __cplusplus >= 201103L
        typedef _Base_const_iterator _Base_iter;
#else
        typedef _Base_iterator _Base_iter;
#endif
        for (_Base_iter __victim = __first.base();
             __victim != __last.base(); ++__victim)


Or just restore the original code which worked fine!

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] __debug::list use C++11 direct initialization

Jonathan Wakely-3
On 17/10/18 17:10 +0100, Jonathan Wakely wrote:

>On 17/10/18 17:03 +0100, Jonathan Wakely wrote:
>>On 09/10/18 07:11 +0200, François Dumont wrote:
>>>Here is the communication for my yesterday's patch which I thought
>>>svn had failed to commit (I had to interrupt it).
>>>
>>>Similarly to what I've done for associative containers here is a
>>>cleanup of the std::__debug::list implementation leveraging more
>>>on C++11 direct initialization.
>>>
>>>I also made sure we use consistent comparison between
>>>iterator/const_iterator in erase and emplace methods.
>>>
>>>2018-10-08  François Dumont <[hidden email]>
>>>
>>>    * include/debug/list (list<>::cbegin()): Use C++11 direct
>>>    initialization.
>>>    (list<>::cend()): Likewise.
>>>    (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
>>>    (list<>::insert(const_iterator, initializer_list<>)): Likewise.
>>>    (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
>>>    (list<>::erase(const_iterator, const_iterator)): Ensure consistent
>>>    iterator comparisons.
>>>    (list<>::splice(const_iterator, list&&, const_iterator,
>>>    const_iterator)): Likewise.
>>>
>>>Tested under Linux x86_64 Debug mode and committed.
>>>
>>>François
>>>
>>
>>>diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
>>>index 8add1d596e0..879e1177497 100644
>>>--- a/libstdc++-v3/include/debug/list
>>>+++ b/libstdc++-v3/include/debug/list
>>>@@ -521,15 +521,17 @@ namespace __debug
>>> // _GLIBCXX_RESOLVE_LIB_DEFECTS
>>> // 151. can't currently clear() empty container
>>> __glibcxx_check_erase_range(__first, __last);
>>>- for (_Base_const_iterator __victim = __first.base();
>>>+ for (__decltype(__first.base()) __victim = __first.base();
>>
>>This change causes the regression.
>>
>>I think the problem is that the decltype is a reference, so every time
>>the loop does ++__victim it changes the iterator stored in __first.
>
>
>This would work:
>
>       typedef typename __decltype(__first)::iterator_type _Base_iter;
> for (_Base_iter __victim = __first.base();
>     __victim != __last.base(); ++__victim)
>
>
>Or simply:
>
>#if __cplusplus >= 201103L
>       typedef _Base_const_iterator _Base_iter;
>#else
>       typedef _Base_iterator _Base_iter;
>#endif
> for (_Base_iter __victim = __first.base();
>     __victim != __last.base(); ++__victim)
>
>
>Or just restore the original code which worked fine!


Unrelated to this change, I'm also seeing some test failures when run
with -O0 -D_GLIBCXX_DEBUG e.g.

make check RUNTESTFLAGS="conformance.exp=23_containers/unordered_*/cliterators.cc --target_board=unix/-O0/-D_GLIBCXX_DEBUG"

There are linker errors:

FAIL: 23_containers/unordered_map/requirements/cliterators.cc (test for excess errors)
Excess errors:
/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_local_iterator.h:332: undefined reference to `__gnu_debug::_Safe_local_iterator_base::_M_attach_single(__gnu_debug::_Safe_sequence_base*, bool)'
/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/safe_local_iterator.h:332: undefined reference to `__gnu_debug::_Safe_local_iterator_base::_M_attach_single(__gnu_debug::_Safe_sequence_base*, bool)'
collect2: error: ld returned 1 exit status
UNRESOLVED: 23_containers/unordered_map/requirements/cliterators.cc compilation failed to produce executable