Any feedback ?
Thanks
On 08/08/21 9:34 pm, François Dumont wrote:
After further testing here a fixed version which imply less changes.
Moreover I already commit the fixes unrelated with this patch.
libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
libstdc++-v3/ChangeLog:
* include/bits/stl_algobase.h (equal): Use runtime-only
_GLIBCXX_DEBUG check.
* include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]:
Include <debug/stl_iterator.h>.
* include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define
debug macros non-empty. Most of
the time do a simple valid_range check.
* include/debug/helper_functions.h: Cleanup comment about
removed _Iter_base.
(__gnu_debug::__valid_range): Add __skip_if_constexpr
parameter and skip check when true
and in a constexpr context.
* include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define
as __glibcxx_assert when only
_GLIBCXX_ASSERTIONS is defined.
(__glibcxx_check_valid_range): Add _SkipIfConstexpr
parameter.
(__glibcxx_check_can_increment_range): Likewise.
* include/debug/safe_iterator.h (__valid_range): Adapt.
* include/debug/safe_local_iterator.h (__valid_range): Adapt.
* testsuite/24_iterators/istream_iterator/1.cc (test01):
Skip iterator increment when
_GLIBCXX_ASSERTIONS is defined.
* testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
* testsuite/25_algorithms/heap/1.cc: Skip operation
complexity checks when _GLIBCXX_ASSERTIONS
is defined.
Ok to commit ?
François
On 06/08/21 4:52 pm, François Dumont wrote:
On 07/06/21 6:25 am, François Dumont wrote:
On 03/06/21 2:31 pm, Jonathan Wakely wrote:
+ }
+
/* Checks that [first, last) is a valid range, and then returns
* __first. This routine is useful when we can't use a separate
* assertion statement because, e.g., we are in a constructor.
@@ -260,8 +279,9 @@ namespace __gnu_debug
inline bool
__check_sorted(const _InputIterator& __first, const
_InputIterator& __last)
{
- return __check_sorted_aux(__first, __last,
- std::__iterator_category(__first));
+ return __skip_debug_runtime_check()
+ || __check_sorted_aux(__first, __last,
+ std::__iterator_category(__first));
Currently this function is never called at all ifndef _GLIBCXX_DEBUG.
With this change, it's going to be present for _GLIBCXX_ASSERTIONS,
and if it isn't inlined it's going to explode the code size.
Some linux distros are already building the entire distro with
_GLIBCXX_ASSERTIONS so I think we need to be quite careful about this
kind of large change affecting every algo.
So maybe we shouldn't enable these checks via _GLIBCXX_ASSERTIONS, but
a new macro.
_GLIBCXX_DEBUG is already rarely used, so will be yet another mode.
So let's forget about all this, thanks.
I eventually wonder if your feedback was limited to the use of
__check_sorted and some other codes perhaps.
So here is another proposal which activate a small subset of the
_GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS but with far less code.
First, the _Error_formatter is not used, the injected checks are
simply using __glibcxx_assert.
Second I reduced the number of accitaved checks, mostly the
__valid_range.
I also enhance the valid_range check for constexpr because sometimes
the normal implementation is good enough to let the compiler diagnose
a potential issue in this context. This is for example the case of
the std::equal implementation whereas the std::copy implementation is
too defensive.
libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
libstdc++-v3/ChangeLog:
* include/bits/stl_algobase.h (equal): Use runtime-only
_GLIBCXX_DEBUG check.
* include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]:
Include <debug/stl_iterator.h>.
* include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define
debug macros non-empty. Most of
the time do a simple valid_range check.
* include/debug/helper_functions.h: Cleanup comment about
removed _Iter_base.
(__valid_range): Add __skip_if_constexpr parameter and
skip check when in a constexpr
context.
* include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define
as __glibcxx_assert when only
_GLIBCXX_ASSERTIONS is defined.
(__glibcxx_check_valid_range): Add _SkipIfConstexpr
parameter.
(__glibcxx_check_can_increment_range): Likewise.
* testsuite/24_iterators/istream_iterator/1.cc (test01):
Skip iterator increment when
_GLIBCXX_ASSERTIONS is defined.
* testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
* testsuite/25_algorithms/heap/1.cc: Skip operation
complexity checks when _GLIBCXX_ASSERTIONS
is defined.
*
testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc:
Fix dg-prune-output reason.
*
testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc:
Likewise.
*
testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc:
Likewise.
*
testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc:
Likewise.
*
testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc:
Likewise.
*
testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc:
Likewise.
The last fixes below are due to the recent changes to the
__glibcxx_assert macro but it is close to the code I am changing so I
prefer to fix those here.
Tested under Linux x86_64 w/o _GLIBCXX_ASSERTIONS.
Ok to commit ?
François
