| Message ID | 926a61ca-121a-39c9-72fa-5327dd9ddc6b@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | __debug::list use C++11 direct initialization | expand |
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 <fdumont@gcc.gnu.org> > > * 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.
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 <fdumont@gcc.gnu.org> >> >> * 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.
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 <fdumont@gcc.gnu.org> >>> >>> * 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.
I've just reverted the controversial changes. 2018-10-18 François Dumont <fdumont@gcc.gnu.org> Partial revert. 2018-10-08 François Dumont <fdumont@gcc.gnu.org> * include/debug/list (list<>::cbegin()): Use C++11 direct initialization. (list<>::cend()): Likewise. (list<>::erase(const_iterator, const_iterator)): Ensure consistent iterator comparisons. (list<>::splice(const_iterator, list&&, const_iterator, const_iterator)): Likewise. Partial revert. 2018-10-15 François Dumont <fdumont@gcc.gnu.org> * include/debug/vector (vector<>::cbegin()): Use C++11 direct initialization. (vector<>::cend()): Likewise. (vector<>::insert(const_iterator, const _Tp&)): Use consistent iterator comparison. (vector<>::erase(const_iterator)): Likewise. (vector<>::erase(const_iterator, const_iterator)): Likewise. François On 10/17/2018 06:10 PM, 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 <fdumont@gcc.gnu.org> >>> >>> * 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! > > . > diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list index 879e1177497..aab146d058b 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 { _Base::begin(), this }; } + { return const_iterator(_Base::begin(), this); } const_iterator cend() const noexcept - { return { _Base::end(), this }; } + { return const_iterator(_Base::end(), this); } const_reverse_iterator crbegin() const noexcept @@ -521,14 +521,13 @@ namespace __debug // _GLIBCXX_RESOLVE_LIB_DEFECTS // 151. can't currently clear() empty container __glibcxx_check_erase_range(__first, __last); - for (__decltype(__first.base()) __victim = __first.base(); + for (_Base_const_iterator __victim = __first.base(); __victim != __last.base(); ++__victim) { - _GLIBCXX_DEBUG_VERIFY( - __victim != __first._M_get_sequence()->_M_base().end(), - _M_message(__gnu_debug::__msg_valid_range) - ._M_iterator(__first, "position") - ._M_iterator(__last, "last")); + _GLIBCXX_DEBUG_VERIFY(__victim != _Base::end(), + _M_message(__gnu_debug::__msg_valid_range) + ._M_iterator(__first, "position") + ._M_iterator(__last, "last")); this->_M_invalidate_if(_Equal(__victim)); } @@ -622,14 +621,13 @@ namespace __debug // We used to perform the splice_alloc check: not anymore, redundant // after implementing the relevant bits of N1599. - for (__decltype(__first.base()) __tmp = __first.base(); + for (_Base_const_iterator __tmp = __first.base(); __tmp != __last.base(); ++__tmp) { - _GLIBCXX_DEBUG_VERIFY( - __tmp != __first._M_get_sequence()->_M_base().end(), - _M_message(__gnu_debug::__msg_valid_range) - ._M_iterator(__first, "first") - ._M_iterator(__last, "last")); + _GLIBCXX_DEBUG_VERIFY(__tmp != _Base::end(), + _M_message(__gnu_debug::__msg_valid_range) + ._M_iterator(__first, "first") + ._M_iterator(__last, "last")); _GLIBCXX_DEBUG_VERIFY(std::__addressof(__x) != this || __tmp != __position.base(), _M_message(__gnu_debug::__msg_splice_overlap) diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector index c11ddbb7048..0b712ba24e9 100644 --- a/libstdc++-v3/include/debug/vector +++ b/libstdc++-v3/include/debug/vector @@ -328,11 +328,11 @@ namespace __debug #if __cplusplus >= 201103L const_iterator cbegin() const noexcept - { return { _Base::begin(), this }; } + { return const_iterator(_Base::begin(), this); } const_iterator cend() const noexcept - { return { _Base::end(), this }; } + { return const_iterator(_Base::end(), this); } const_reverse_iterator crbegin() const noexcept @@ -542,8 +542,7 @@ namespace __debug { __glibcxx_check_insert(__position); bool __realloc = this->_M_requires_reallocation(this->size() + 1); - difference_type __offset - = __position.base() - __position._M_get_sequence()->_M_base().begin(); + difference_type __offset = __position.base() - _Base::begin(); _Base_iterator __res = _Base::insert(__position.base(), __x); if (__realloc) this->_M_invalidate_all(); @@ -662,8 +661,7 @@ namespace __debug #endif { __glibcxx_check_erase(__position); - difference_type __offset - = __position.base() - __position._M_get_sequence()->_M_base().begin(); + difference_type __offset = __position.base() - _Base::begin(); _Base_iterator __res = _Base::erase(__position.base()); this->_M_invalidate_after_nth(__offset); return iterator(__res, this); @@ -682,8 +680,7 @@ namespace __debug if (__first.base() != __last.base()) { - difference_type __offset = - __first.base() - __first._M_get_sequence()->_M_base().begin(); + difference_type __offset = __first.base() - _Base::begin(); _Base_iterator __res = _Base::erase(__first.base(), __last.base()); this->_M_invalidate_after_nth(__offset);
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(); __victim != __last.base(); ++__victim) { - _GLIBCXX_DEBUG_VERIFY(__victim != _Base::end(), - _M_message(__gnu_debug::__msg_valid_range) - ._M_iterator(__first, "position") - ._M_iterator(__last, "last")); + _GLIBCXX_DEBUG_VERIFY( + __victim != __first._M_get_sequence()->_M_base().end(), + _M_message(__gnu_debug::__msg_valid_range) + ._M_iterator(__first, "position") + ._M_iterator(__last, "last")); this->_M_invalidate_if(_Equal(__victim)); } + return iterator(_Base::erase(__first.base(), __last.base()), this); } @@ -586,7 +588,7 @@ namespace __debug ._M_iterator(__i, "__i")); _GLIBCXX_DEBUG_VERIFY(__i._M_attached_to(std::__addressof(__x)), _M_message(__gnu_debug::__msg_splice_other) - ._M_iterator(__i, "__i")._M_sequence(__x, "__x")); + ._M_iterator(__i, "__i")._M_sequence(__x, "__x")); // _GLIBCXX_RESOLVE_LIB_DEFECTS // 250. splicing invalidates iterators @@ -620,19 +622,21 @@ namespace __debug // We used to perform the splice_alloc check: not anymore, redundant // after implementing the relevant bits of N1599. - for (_Base_const_iterator __tmp = __first.base(); + for (__decltype(__first.base()) __tmp = __first.base(); __tmp != __last.base(); ++__tmp) { - _GLIBCXX_DEBUG_VERIFY(__tmp != _Base::end(), - _M_message(__gnu_debug::__msg_valid_range) - ._M_iterator(__first, "first") - ._M_iterator(__last, "last")); + _GLIBCXX_DEBUG_VERIFY( + __tmp != __first._M_get_sequence()->_M_base().end(), + _M_message(__gnu_debug::__msg_valid_range) + ._M_iterator(__first, "first") + ._M_iterator(__last, "last")); _GLIBCXX_DEBUG_VERIFY(std::__addressof(__x) != this || __tmp != __position.base(), _M_message(__gnu_debug::__msg_splice_overlap) ._M_iterator(__tmp, "position") ._M_iterator(__first, "first") ._M_iterator(__last, "last")); + // _GLIBCXX_RESOLVE_LIB_DEFECTS // 250. splicing invalidates iterators this->_M_transfer_from_if(__x, _Equal(__tmp));