[_GLIBCXX_DEBUG] Remove useless checks

Message ID dd643693-5e73-cd9f-ae2e-541d253985d0@gmail.com
State New
Headers
Series [_GLIBCXX_DEBUG] Remove useless checks |

Commit Message

François Dumont Jan. 23, 2023, 6:02 a.m. UTC
  libstdc++: [_GLIBCXX_DEBUG] Remove useless constructor checks

     Creating a safe iterator from a normal iterator is done within the 
library where we
     already know that it is done correctly. The rare situation where a 
user would use safe
     iterators for his own purpose is non-Standard code so outside 
_GLIBCXX_DEBUG scope. For
     those reasons the __msg_init_singular is useless and can be removed.

     Additionally in the copy constructor used for post-increment and 
post-decrement operators
     the __msg_init_copy_singular check can also be ommitted because of 
the preliminary
     __msg_bad_inc and __msg_bad_dec checks.

     libstdc++-v3/ChangeLog:

             * include/debug/safe_iterator.h 
(_Safe_iterator<>::_Unsafe_call): New.
             (_Safe_iterator(const _Safe_iterator&, _Unsafe_call): New.
             (_Safe_iterator::operator++(int)): Use latter.
             (_Safe_iterator::operator--(int)): Likewise.
             (_Safe_iterator(_Iterator, const _Safe_sequence_base*)): 
Remove !_M_insular()
             check.
             * include/debug/safe_local_iterator.h 
(_Safe_local_iterator<>::_Unsafe_call):
             New.
             (_Safe_local_iterator(const _Safe_local_iterator&, 
_Unsafe_call): New.
             (_Safe_local_iterator::operator++(int)): Use latter.
             * src/c++11/debug.cc (_S_debug_messages): Add as comment 
the _Debug_msg_id
             entry associated to the array entry.

Tested under Linux x64.

Ok to commit ?

François
  

Comments

Jonathan Wakely Jan. 23, 2023, 9:22 a.m. UTC | #1
On Mon, 23 Jan 2023 at 06:02, François Dumont via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
>      libstdc++: [_GLIBCXX_DEBUG] Remove useless constructor checks
>
>      Creating a safe iterator from a normal iterator is done within the
> library where we
>      already know that it is done correctly. The rare situation where a
> user would use safe
>      iterators for his own purpose is non-Standard code so outside
> _GLIBCXX_DEBUG scope. For
>      those reasons the __msg_init_singular is useless and can be removed.
>
>      Additionally in the copy constructor used for post-increment and
> post-decrement operators
>      the __msg_init_copy_singular check can also be ommitted because of
> the preliminary
>      __msg_bad_inc and __msg_bad_dec checks.
>
>      libstdc++-v3/ChangeLog:
>
>              * include/debug/safe_iterator.h
> (_Safe_iterator<>::_Unsafe_call): New.

I don't like the name "unsafe call". Why is it unsafe? As you say
above, we don't need to check because we know that it's only called in
a context where it's safe. Can we call it _Unchecked instead of
_Unsafe_call? That seems like a more accurate description of the
behaviour.


>              (_Safe_iterator(const _Safe_iterator&, _Unsafe_call): New.
>              (_Safe_iterator::operator++(int)): Use latter.
>              (_Safe_iterator::operator--(int)): Likewise.
>              (_Safe_iterator(_Iterator, const _Safe_sequence_base*)):
> Remove !_M_insular()
>              check.
>              * include/debug/safe_local_iterator.h
> (_Safe_local_iterator<>::_Unsafe_call):
>              New.
>              (_Safe_local_iterator(const _Safe_local_iterator&,
> _Unsafe_call): New.
>              (_Safe_local_iterator::operator++(int)): Use latter.
>              * src/c++11/debug.cc (_S_debug_messages): Add as comment
> the _Debug_msg_id
>              entry associated to the array entry.

These comments are a great idea, thanks.

If you agree with the _Unchecked name, OK to commit with that change.
  
François Dumont Jan. 23, 2023, 6:15 p.m. UTC | #2
On 23/01/23 10:22, Jonathan Wakely wrote:
> On Mon, 23 Jan 2023 at 06:02, François Dumont via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>>       libstdc++: [_GLIBCXX_DEBUG] Remove useless constructor checks
>>
>>       Creating a safe iterator from a normal iterator is done within the
>> library where we
>>       already know that it is done correctly. The rare situation where a
>> user would use safe
>>       iterators for his own purpose is non-Standard code so outside
>> _GLIBCXX_DEBUG scope. For
>>       those reasons the __msg_init_singular is useless and can be removed.
>>
>>       Additionally in the copy constructor used for post-increment and
>> post-decrement operators
>>       the __msg_init_copy_singular check can also be ommitted because of
>> the preliminary
>>       __msg_bad_inc and __msg_bad_dec checks.
>>
>>       libstdc++-v3/ChangeLog:
>>
>>               * include/debug/safe_iterator.h
>> (_Safe_iterator<>::_Unsafe_call): New.
> I don't like the name "unsafe call". Why is it unsafe? As you say
> above, we don't need to check because we know that it's only called in
> a context where it's safe. Can we call it _Unchecked instead of
> _Unsafe_call? That seems like a more accurate description of the
> behaviour.
>
>
>>               (_Safe_iterator(const _Safe_iterator&, _Unsafe_call): New.
>>               (_Safe_iterator::operator++(int)): Use latter.
>>               (_Safe_iterator::operator--(int)): Likewise.
>>               (_Safe_iterator(_Iterator, const _Safe_sequence_base*)):
>> Remove !_M_insular()
>>               check.
>>               * include/debug/safe_local_iterator.h
>> (_Safe_local_iterator<>::_Unsafe_call):
>>               New.
>>               (_Safe_local_iterator(const _Safe_local_iterator&,
>> _Unsafe_call): New.
>>               (_Safe_local_iterator::operator++(int)): Use latter.
>>               * src/c++11/debug.cc (_S_debug_messages): Add as comment
>> the _Debug_msg_id
>>               entry associated to the array entry.
> These comments are a great idea, thanks.
>
> If you agree with the _Unchecked name, OK to commit with that change.
>
It's unsafe because it's unchecked so _Unchecked is fine for me too :-)

Committed with the requested change.

Thanks
  
Jonathan Wakely Jan. 23, 2023, 6:25 p.m. UTC | #3
On Mon, 23 Jan 2023 at 18:15, François Dumont <frs.dumont@gmail.com> wrote:
>
> On 23/01/23 10:22, Jonathan Wakely wrote:
> > On Mon, 23 Jan 2023 at 06:02, François Dumont via Libstdc++
> > <libstdc++@gcc.gnu.org> wrote:
> >>       libstdc++: [_GLIBCXX_DEBUG] Remove useless constructor checks
> >>
> >>       Creating a safe iterator from a normal iterator is done within the
> >> library where we
> >>       already know that it is done correctly. The rare situation where a
> >> user would use safe
> >>       iterators for his own purpose is non-Standard code so outside
> >> _GLIBCXX_DEBUG scope. For
> >>       those reasons the __msg_init_singular is useless and can be removed.
> >>
> >>       Additionally in the copy constructor used for post-increment and
> >> post-decrement operators
> >>       the __msg_init_copy_singular check can also be ommitted because of
> >> the preliminary
> >>       __msg_bad_inc and __msg_bad_dec checks.
> >>
> >>       libstdc++-v3/ChangeLog:
> >>
> >>               * include/debug/safe_iterator.h
> >> (_Safe_iterator<>::_Unsafe_call): New.
> > I don't like the name "unsafe call". Why is it unsafe? As you say
> > above, we don't need to check because we know that it's only called in
> > a context where it's safe. Can we call it _Unchecked instead of
> > _Unsafe_call? That seems like a more accurate description of the
> > behaviour.
> >
> >
> >>               (_Safe_iterator(const _Safe_iterator&, _Unsafe_call): New.
> >>               (_Safe_iterator::operator++(int)): Use latter.
> >>               (_Safe_iterator::operator--(int)): Likewise.
> >>               (_Safe_iterator(_Iterator, const _Safe_sequence_base*)):
> >> Remove !_M_insular()
> >>               check.
> >>               * include/debug/safe_local_iterator.h
> >> (_Safe_local_iterator<>::_Unsafe_call):
> >>               New.
> >>               (_Safe_local_iterator(const _Safe_local_iterator&,
> >> _Unsafe_call): New.
> >>               (_Safe_local_iterator::operator++(int)): Use latter.
> >>               * src/c++11/debug.cc (_S_debug_messages): Add as comment
> >> the _Debug_msg_id
> >>               entry associated to the array entry.
> > These comments are a great idea, thanks.
> >
> > If you agree with the _Unchecked name, OK to commit with that change.
> >
> It's unsafe because it's unchecked so _Unchecked is fine for me too :-)

But it doesn't need to be checked, because we know it's safe in all
the places that it's used. So it's not unsafe :-)

In some industries the words "safe" and "unsafe" have special meaning.
Calling something "unsafe" when actually it's "safe by construction"
will just raise red flags and unnecessary pain if an automated audit
scans for the word "unsafe". Some poor person will have to fill out a
form to say "it's not unsafe, it's just called that in the code" to be
able to use the code.

> Committed with the requested change.

Great, thanks.
  

Patch

diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index f364477a00c..570c826649f 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -129,6 +129,12 @@  namespace __gnu_debug
 	typename _Sequence::_Base::iterator,
 	typename _Sequence::_Base::const_iterator>::__type _OtherIterator;
 
+      struct _Unsafe_call { };
+
+      _Safe_iterator(const _Safe_iterator& __x, _Unsafe_call) _GLIBCXX_NOEXCEPT
+      : _Iter_base(__x.base()), _Safe_base()
+      { _M_attach(__x._M_sequence); }
+
     public:
       typedef _Iterator					iterator_type;
       typedef typename _Traits::iterator_category	iterator_category;
@@ -154,11 +160,7 @@  namespace __gnu_debug
       _Safe_iterator(_Iterator __i, const _Safe_sequence_base* __seq)
       _GLIBCXX_NOEXCEPT
       : _Iter_base(__i), _Safe_base(__seq, _S_constant())
-      {
-	_GLIBCXX_DEBUG_VERIFY(!this->_M_singular(),
-			      _M_message(__msg_init_singular)
-			      ._M_iterator(*this, "this"));
-      }
+      { }
 
       /**
        * @brief Copy construction.
@@ -339,7 +341,7 @@  namespace __gnu_debug
 	_GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(),
 			      _M_message(__msg_bad_inc)
 			      ._M_iterator(*this, "this"));
-	_Safe_iterator __ret = *this;
+	_Safe_iterator __ret(*this, _Unsafe_call());
 	++*this;
 	return __ret;
       }
@@ -514,6 +516,13 @@  namespace __gnu_debug
     protected:
       typedef typename _Safe_base::_OtherIterator _OtherIterator;
 
+      typedef typename _Safe_base::_Unsafe_call _Unsafe_call;
+
+      _Safe_iterator(const _Safe_iterator& __x,
+		     _Unsafe_call __unsafe_call) _GLIBCXX_NOEXCEPT
+	: _Safe_base(__x, __unsafe_call)
+      { }
+
     public:
       /// @post the iterator is singular and unattached
       _Safe_iterator() _GLIBCXX_NOEXCEPT { }
@@ -596,7 +605,7 @@  namespace __gnu_debug
 	_GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(),
 			      _M_message(__msg_bad_inc)
 			      ._M_iterator(*this, "this"));
-	_Safe_iterator __ret = *this;
+	_Safe_iterator __ret(*this, _Unsafe_call());
 	++*this;
 	return __ret;
       }
@@ -627,7 +636,7 @@  namespace __gnu_debug
 	_GLIBCXX_DEBUG_VERIFY(this->_M_decrementable(),
 			      _M_message(__msg_bad_dec)
 			      ._M_iterator(*this, "this"));
-	_Safe_iterator __ret = *this;
+	_Safe_iterator __ret(*this, _Unsafe_call());
 	--*this;
 	return __ret;
       }
@@ -653,6 +662,12 @@  namespace __gnu_debug
       typedef _Safe_iterator<_OtherIterator, _Sequence,
 			     std::random_access_iterator_tag> _OtherSelf;
 
+      typedef typename _Safe_base::_Unsafe_call _Unsafe_call;
+      _Safe_iterator(const _Safe_iterator& __x,
+		     _Unsafe_call __unsafe_call) _GLIBCXX_NOEXCEPT
+	: _Safe_base(__x, __unsafe_call)
+      { }
+
     public:
       typedef typename _Safe_base::difference_type	difference_type;
       typedef typename _Safe_base::reference		reference;
@@ -744,7 +759,7 @@  namespace __gnu_debug
 	_GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(),
 			      _M_message(__msg_bad_inc)
 			      ._M_iterator(*this, "this"));
-	_Safe_iterator __ret = *this;
+	_Safe_iterator __ret(*this, _Unsafe_call());
 	++*this;
 	return __ret;
       }
@@ -771,7 +786,7 @@  namespace __gnu_debug
 	_GLIBCXX_DEBUG_VERIFY(this->_M_decrementable(),
 			      _M_message(__msg_bad_dec)
 			      ._M_iterator(*this, "this"));
-	_Safe_iterator __ret = *this;
+	_Safe_iterator __ret(*this, _Unsafe_call());
 	--*this;
 	return __ret;
       }
diff --git a/libstdc++-v3/include/debug/safe_local_iterator.h b/libstdc++-v3/include/debug/safe_local_iterator.h
index 624f0ddad0f..feb54d082d4 100644
--- a/libstdc++-v3/include/debug/safe_local_iterator.h
+++ b/libstdc++-v3/include/debug/safe_local_iterator.h
@@ -84,6 +84,13 @@  namespace __gnu_debug
       typedef _Safe_local_iterator _Self;
       typedef _Safe_local_iterator<_OtherIterator, _Sequence> _OtherSelf;
 
+      struct _Unsafe_call { };
+
+      _Safe_local_iterator(const _Safe_local_iterator& __x,
+			   _Unsafe_call) noexcept
+      : _Iter_base(__x.base())
+      { _M_attach(__x._M_sequence); }
+
     public:
       typedef _Iterator					iterator_type;
       typedef typename _Traits::iterator_category	iterator_category;
@@ -104,11 +111,7 @@  namespace __gnu_debug
        */
       _Safe_local_iterator(_Iterator __i, const _Safe_sequence_base* __cont)
       : _Iter_base(__i), _Safe_base(__cont, _S_constant())
-      {
-	_GLIBCXX_DEBUG_VERIFY(!this->_M_singular(),
-			      _M_message(__msg_init_singular)
-			      ._M_iterator(*this, "this"));
-      }
+      { }
 
       /**
        * @brief Copy construction.
@@ -282,7 +285,7 @@  namespace __gnu_debug
 	_GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(),
 			      _M_message(__msg_bad_inc)
 			      ._M_iterator(*this, "this"));
-	_Safe_local_iterator __ret = *this;
+	_Safe_local_iterator __ret(*this, _Unsafe_call{});
 	++*this;
 	return __ret;
       }
diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc
index d4ee5fa12dc..926e8be6122 100644
--- a/libstdc++-v3/src/c++11/debug.cc
+++ b/libstdc++-v3/src/c++11/debug.cc
@@ -181,86 +181,139 @@  namespace __gnu_debug
   const char* const _S_debug_messages[] =
   {
     // General Checks
+    // __msg_valid_range
     "function requires a valid iterator range [%1.name;, %2.name;)",
+    // __msg_insert_singular
     "attempt to insert into container with a singular iterator",
+    // __msg_insert_different
     "attempt to insert into container with an iterator"
     " from a different container",
+    // __msg_erase_bad
     "attempt to erase from container with a %2.state; iterator",
+    // __msg_erase_different
     "attempt to erase from container with an iterator"
     " from a different container",
+    // __msg_subscript_oob
     "attempt to subscript container with out-of-bounds index %2;,"
     " but container only holds %3; elements",
+    // __msg_empty
     "attempt to access an element in an empty container",
+    // __msg_unpartitioned
     "elements in iterator range [%1.name;, %2.name;)"
     " are not partitioned by the value %3;",
+    // __msg_unpartitioned_pred
     "elements in iterator range [%1.name;, %2.name;)"
     " are not partitioned by the predicate %3; and value %4;",
+    // __msg_unsorted
     "elements in iterator range [%1.name;, %2.name;) are not sorted",
+    // __msg_unsorted_pred
     "elements in iterator range [%1.name;, %2.name;)"
     " are not sorted according to the predicate %3;",
+    // __msg_not_heap
     "elements in iterator range [%1.name;, %2.name;) do not form a heap",
+    // __msg_not_heap_pred
     "elements in iterator range [%1.name;, %2.name;)"
     " do not form a heap with respect to the predicate %3;",
     // std::bitset checks
+    // __msg_bad_bitset_write
     "attempt to write through a singular bitset reference",
+    // __msg_bad_bitset_read
     "attempt to read from a singular bitset reference",
+    // __msg_bad_bitset_flip
     "attempt to flip a singular bitset reference",
     // std::list checks
+    // __msg_self_splice
     "attempt to splice a list into itself",
+    // __msg_splice_alloc
     "attempt to splice lists with unequal allocators",
+    // __msg_splice_bad
     "attempt to splice elements referenced by a %1.state; iterator",
+    // __msg_splice_other
     "attempt to splice an iterator from a different container",
+    // __msg_splice_overlap
     "splice destination %1.name;"
     " occurs within source range [%2.name;, %3.name;)",
     // iterator checks
+    // __msg_init_singular
     "attempt to initialize an iterator that will immediately become singular",
+    // __msg_init_copy_singular
     "attempt to copy-construct an iterator from a singular iterator",
+    // __msg_init_const_singular
     "attempt to construct a constant iterator"
     " from a singular mutable iterator",
+    // __msg_copy_singular
     "attempt to copy from a singular iterator",
+    // __msg_bad_deref
     "attempt to dereference a %1.state; iterator",
+    // __msg_bad_inc
     "attempt to increment a %1.state; iterator",
+    // __msg_bad_dec
     "attempt to decrement a %1.state; iterator",
+    // __msg_iter_subscript_oob
     "attempt to subscript a %1.state; iterator %2; step from"
     " its current position, which falls outside its dereferenceable range",
+    // __msg_advance_oob
     "attempt to advance a %1.state; iterator %2; steps,"
     " which falls outside its valid range",
+    // __msg_retreat_oob
     "attempt to retreat a %1.state; iterator %2; steps,"
     " which falls outside its valid range",
+    // __msg_iter_compare_bad
     "attempt to compare a %1.state; iterator to a %2.state; iterator",
+    // __msg_compare_different
     "attempt to compare iterators from different sequences",
+    // __msg_iter_order_bad
     "attempt to order a %1.state; iterator to a %2.state; iterator",
+    // __msg_order_different
     "attempt to order iterators from different sequences",
+    // __msg_distance_bad
     "attempt to compute the difference between a %1.state;"
     " iterator to a %2.state; iterator",
+    // __msg_distance_different
     "attempt to compute the different between two iterators"
     " from different sequences",
     // istream_iterator
+    // __msg_deref_istream
     "attempt to dereference an end-of-stream istream_iterator",
+    // __msg_inc_istream
     "attempt to increment an end-of-stream istream_iterator",
     // ostream_iterator
+    // __msg_output_ostream
     "attempt to output via an ostream_iterator with no associated stream",
     // istreambuf_iterator
+    // __msg_deref_istreambuf
     "attempt to dereference an end-of-stream istreambuf_iterator"
     " (this is a GNU extension)",
+    // __msg_inc_istreambuf
     "attempt to increment an end-of-stream istreambuf_iterator",
     // std::forward_list
+    // __msg_insert_after_end
     "attempt to insert into container after an end iterator",
+    // __msg_erase_after_bad
     "attempt to erase from container after a %2.state; iterator not followed"
     " by a dereferenceable one",
+    // __msg_valid_range2
     "function requires a valid iterator range (%2.name;, %3.name;)"
     ", \"%2.name;\" shall be before and not equal to \"%3.name;\"",
     // std::unordered_container::local_iterator
+    // __msg_local_iter_compare_bad
     "attempt to compare local iterators from different unordered container"
     " buckets",
+    // __msg_non_empty_range
     "function requires a non-empty iterator range [%1.name;, %2.name;)",
+    // __msg_self_move_assign
     "attempt to self move assign",
+    // __msg_bucket_index_oob
     "attempt to access container with out-of-bounds bucket index %2;,"
     " container only holds %3; buckets",
+    // __msg_valid_load_factor
     "load factor shall be positive",
+    // __msg_equal_allocs
     "allocators must be equal",
+    // __msg_insert_range_from_self
     "attempt to insert with an iterator range [%1.name;, %2.name;) from this"
     " container",
+    // __msg_irreflexive_ordering
     "comparison doesn't meet irreflexive requirements, assert(!(a < a))"
   };