diff mbox series

Use C++11 direct init in __debug::forward_list

Message ID a7545824-cd01-622e-a328-8abcda996fc3@gmail.com
State New
Headers show
Series Use C++11 direct init in __debug::forward_list | expand

Commit Message

François Dumont Oct. 11, 2018, 8:46 p.m. UTC 
This patch makes extensive use of C++11 direct init in 
__debug::forward_list.

Doing so I also try to detect useless creation of safe iterators in 
debug implementation. In __debug::forward_list there are severals but I 
wonder if it is worth fixing those. Most of them are like this:

       void
       splice_after(const_iterator __pos, forward_list& __list)
       { splice_after(__pos, std::move(__list)); }

__pos is copied.

Do you think I shouldn't care, gcc will optimize it ?

I wonder if it would be ok in debug implementation to use this kind of 
signature:

void splice_after(const const_iterator& __pos, forward_list& __list)

Iterator taken as rvalue reference ?

I guess it is not Standard conformant so not correct but maybe I could 
add a private _M_splice_after with this signature.

     * include/debug/forward_list
     (forward_list<>::before_begin()): Use C++11 direct initialization.
     (forward_list<>::begin()): Likewise.
     (forward_list<>::end()): Likewise.
     (forward_list<>::cbefore_begin()): Likewise.
     (forward_list<>::cbegin()): Likewise.
     (forward_list<>::cend()): Likewise.
     (forward_list<>::emplace_after<>(const_iterator, _Args&&...)): 
Likewise.
     (forward_list<>::insert_after(const_iterator, const _Tp&)): Likewise.
     (forward_list<>::insert_after(const_iterator, _Tp&&)): Likewise.
     (forward_list<>::insert_after(const_iterator, size_type, const _Tp&)):
     Likewise.
     (forward_list<>::insert_after(const_iterator, initializer_list<>)):
     Likewise.
     (forward_list<>::erase_after(const_iterator)): Likewise.
     (forward_list<>::erase_after(const_iterator, const_iterator)): Likewise
     and ensure consistent iterator comparison.

Tested under Linux x86_64, Debug mode and committed.

François

Comments

Jonathan Wakely Oct. 15, 2018, 9:58 a.m. UTC | #1 
On 11/10/18 22:46 +0200, François Dumont wrote:
>This patch makes extensive use of C++11 direct init in 
>__debug::forward_list.
>
>Doing so I also try to detect useless creation of safe iterators in 
>debug implementation. In __debug::forward_list there are severals but 
>I wonder if it is worth fixing those. Most of them are like this:
>
>      void
>      splice_after(const_iterator __pos, forward_list& __list)
>      { splice_after(__pos, std::move(__list)); }
>
>__pos is copied.
>
>Do you think I shouldn't care, gcc will optimize it ?

I think the _Safe_iterator construction/destruction is too complex to
be optimised away (it locks a mutex, doesn't it?).

Normally I'd say you could use std::move(__pos) but IIRC that's even
more expensive than a copy, as it locks two mutexes.

>I wonder if it would be ok in debug implementation to use this kind of 
>signature:
>
>void splice_after(const const_iterator& __pos, forward_list& __list)
>
>Iterator taken as rvalue reference ?
>
>I guess it is not Standard conformant so not correct but maybe I could 
>add a private _M_splice_after with this signature.

It doesn't seem worthwhile to me.
François Dumont Oct. 15, 2018, 8:13 p.m. UTC | #2 
On 10/15/2018 11:58 AM, Jonathan Wakely wrote:
> On 11/10/18 22:46 +0200, François Dumont wrote:
>> This patch makes extensive use of C++11 direct init in 
>> __debug::forward_list.
>>
>> Doing so I also try to detect useless creation of safe iterators in 
>> debug implementation. In __debug::forward_list there are severals but 
>> I wonder if it is worth fixing those. Most of them are like this:
>>
>>       void
>>       splice_after(const_iterator __pos, forward_list& __list)
>>       { splice_after(__pos, std::move(__list)); }
>>
>> __pos is copied.
>>
>> Do you think I shouldn't care, gcc will optimize it ?
>
> I think the _Safe_iterator construction/destruction is too complex to
> be optimised away (it locks a mutex, doesn't it?).

Yes it does, I also would be surprised if gcc was able to optimize it away.

>
> Normally I'd say you could use std::move(__pos) but IIRC that's even
> more expensive than a copy, as it locks two mutexes.
>
>> I wonder if it would be ok in debug implementation to use this kind 
>> of signature:
>>
>> void splice_after(const const_iterator& __pos, forward_list& __list)
>>
>> Iterator taken as rvalue reference ?
>>
>> I guess it is not Standard conformant so not correct but maybe I 
>> could add a private _M_splice_after with this signature.
>
> It doesn't seem worthwhile to me.
>
>
Ok, I'll leave it this way.

Thanks,

François
diff mbox series

Patch

diff --git a/libstdc++-v3/include/debug/forward_list b/libstdc++-v3/include/debug/forward_list
index e542447badd..c9744eda55a 100644
--- a/libstdc++-v3/include/debug/forward_list
+++ b/libstdc++-v3/include/debug/forward_list
@@ -316,39 +316,39 @@  namespace __debug
 
       iterator
       before_begin() noexcept
-      { return iterator(_Base::before_begin(), this); }
+      { return { _Base::before_begin(), this }; }
 
       const_iterator
       before_begin() const noexcept
-      { return const_iterator(_Base::before_begin(), this); }
+      { return { _Base::before_begin(), this }; }
 
       iterator
       begin() noexcept
-      { return iterator(_Base::begin(), this); }
+      { return { _Base::begin(), this }; }
 
       const_iterator
       begin() const noexcept
-      { return const_iterator(_Base::begin(), this); }
+      { return { _Base::begin(), this }; }
 
       iterator
       end() noexcept
-      { return iterator(_Base::end(), this); }
+      { return { _Base::end(), this }; }
 
       const_iterator
       end() const noexcept
-      { return const_iterator(_Base::end(), this); }
+      { return { _Base::end(), this }; }
 
       const_iterator
       cbegin() const noexcept
-      { return const_iterator(_Base::cbegin(), this); }
+      { return { _Base::cbegin(), this }; }
 
       const_iterator
       cbefore_begin() const noexcept
-      { return const_iterator(_Base::cbefore_begin(), this); }
+      { return { _Base::cbefore_begin(), this }; }
 
       const_iterator
       cend() const noexcept
-      { return const_iterator(_Base::cend(), this); }
+      { return { _Base::cend(), this }; }
 
       using _Base::empty;
       using _Base::max_size;
@@ -388,32 +388,30 @@  namespace __debug
 	emplace_after(const_iterator __pos, _Args&&... __args)
 	{
 	  __glibcxx_check_insert_after(__pos);
-	  return iterator(_Base::emplace_after(__pos.base(),
+	  return { _Base::emplace_after(__pos.base(),
 					std::forward<_Args>(__args)...),
-			  this);
+		   this };
        	}
 
       iterator
       insert_after(const_iterator __pos, const _Tp& __val)
       {
 	__glibcxx_check_insert_after(__pos);
-	return iterator(_Base::insert_after(__pos.base(), __val), this);
+	return { _Base::insert_after(__pos.base(), __val), this };
       }
 
       iterator
       insert_after(const_iterator __pos, _Tp&& __val)
       {
 	__glibcxx_check_insert_after(__pos);
-	return iterator(_Base::insert_after(__pos.base(), std::move(__val)),
-		       	this);
+	return { _Base::insert_after(__pos.base(), std::move(__val)), this };
       }
 
       iterator
       insert_after(const_iterator __pos, size_type __n, const _Tp& __val)
       {
 	__glibcxx_check_insert_after(__pos);
-	return iterator(_Base::insert_after(__pos.base(), __n, __val),
-		       	this);
+	return { _Base::insert_after(__pos.base(), __n, __val), this };
       }
 
       template<typename _InputIterator,
@@ -441,7 +439,7 @@  namespace __debug
       insert_after(const_iterator __pos, std::initializer_list<_Tp> __il)
       {
 	__glibcxx_check_insert_after(__pos);
-	return iterator(_Base::insert_after(__pos.base(), __il), this);
+	return { _Base::insert_after(__pos.base(), __il), this };
       }
 
     private:
@@ -458,7 +456,7 @@  namespace __debug
       erase_after(const_iterator __pos)
       {
 	__glibcxx_check_erase_after(__pos);
-	return iterator(_M_erase_after(__pos.base()), this);
+	return { _M_erase_after(__pos.base()), this };
       }
 
       iterator
@@ -468,7 +466,7 @@  namespace __debug
 	for (_Base_const_iterator __victim = std::next(__pos.base());
 	    __victim != __last.base(); ++__victim)
 	  {
-	    _GLIBCXX_DEBUG_VERIFY(__victim != _Base::end(),
+	    _GLIBCXX_DEBUG_VERIFY(__victim != _Base::cend(),
 				  _M_message(__gnu_debug::__msg_valid_range2)
 				  ._M_sequence(*this, "this")
 				  ._M_iterator(__pos, "pos")
@@ -476,7 +474,8 @@  namespace __debug
 	    this->_M_invalidate_if([__victim](_Base_const_iterator __it)
 	      { return __it == __victim; });
 	  }
-	return iterator(_Base::erase_after(__pos.base(), __last.base()), this);
+
+	return { _Base::erase_after(__pos.base(), __last.base()), this };
       }
 
       void