[PATCH] D150264: [libcxx] Add strict weak ordering checks to sorting algorithms

[Nikolas Klauser via Phabricator via llvm-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/sort.h:704
   typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
   using _Comp_ref = __comp_ref_type<_Compare>;
   // Upper bound for using insertion sort for sorting.
----------------
ldionne wrote:
> ldionne wrote:
> > Sorry for being so late to the party, but I think there's a problem here. We should decide whether we want to use these added facilities or `__comp_ref_type` to catch these kinds of issues. Right now, we have a weird situation where both can potentially be used at the same time. This means that we have the following mechanisms in place and enabled independently:
> > 
> > 1. The assertions I added that ensure that we would not dereference iterators out-of-bounds inside `std::sort`. Those are arguably hardening assertions, not only debug mode assertions.
> > 2. The existing `__comp_ref_type` mechanism which aims to catch invalid strict weak orders at the predicate level.
> > 3. The new mechanism introduced by this patch.
> > 
> > Is there a reason why we'd want to have these three severely overlapping mechanisms (and especially within the same algorithm)?
> CC @var-const 
I think they are complementary. All of them have their strengths and weaknesses.

- The assertions are relatively light weight, but miss a lot of the strict weak ordering issues. As you said yourself, these can be considered hardening asserts.
- The `__comp_ref_type` catches a lot more issues, but can't check across multiple pairs.
- This new check is //very// expensive, making it only suitable for a subrange and only in debug mode, but it catches a lot of the strict weak ordering issues.

I think the assertions aren't useful to combine with `__comp_ref_type` and the new checks, but also don't hurt. Having both for larger ranges makes sense, since the `__comp_ref_type` can check the whole range.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150264/new/

https://reviews.llvm.org/D150264