* [PATCH][_GLIBCXX_DEBUG] Remove useless checks
@ 2023-01-23 6:02 François Dumont
2023-01-23 9:22 ` Jonathan Wakely
0 siblings, 1 reply; 4+ messages in thread
From: François Dumont @ 2023-01-23 6:02 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]
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
[-- Attachment #2: useless_debug_check.patch --]
[-- Type: text/x-patch, Size: 11170 bytes --]
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))"
};
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH][_GLIBCXX_DEBUG] Remove useless checks
2023-01-23 6:02 [PATCH][_GLIBCXX_DEBUG] Remove useless checks François Dumont
@ 2023-01-23 9:22 ` Jonathan Wakely
2023-01-23 18:15 ` François Dumont
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Wakely @ 2023-01-23 9:22 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] Remove useless checks
2023-01-23 9:22 ` Jonathan Wakely
@ 2023-01-23 18:15 ` François Dumont
2023-01-23 18:25 ` Jonathan Wakely
0 siblings, 1 reply; 4+ messages in thread
From: François Dumont @ 2023-01-23 18:15 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][_GLIBCXX_DEBUG] Remove useless checks
2023-01-23 18:15 ` François Dumont
@ 2023-01-23 18:25 ` Jonathan Wakely
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Wakely @ 2023-01-23 18:25 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-23 18:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23 6:02 [PATCH][_GLIBCXX_DEBUG] Remove useless checks François Dumont
2023-01-23 9:22 ` Jonathan Wakely
2023-01-23 18:15 ` François Dumont
2023-01-23 18:25 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).