[PATCH] D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D39245#907527, @davide wrote:

> In https://reviews.llvm.org/D39245#907426, @mgrang wrote:
>
> > In https://reviews.llvm.org/D39245#907221, @chandlerc wrote:
> >
> > > I don't really like the ever growing set of configuration macros here.
> > >
> > > I would much prefer we have one ABI-breaking check macro and just use that for everything here...
> >
> >
> > Thanks @chandlerc. I am fine with enabling this based on LLVM_ABI_BREAKING_CHECKS. However, this macro is ON by default for +asserts builds. This means all existing +asserts bots would start failing as soon as this patch gets merged since there are about 15 unit test failures. Do we want to gate patches if they show up sort order failures uncovered by shuffling?
> >  My idea was to control this via a separate macro so that we can isolate shuffle+sort failures into a separate bot which may make it easier to fix without gating the regular flow. Thoughts?
>
>
> I agree with Chandler that we should avoid the proliferation of macros and use ABI_BREAKING_CHECKS instead.


I'm actually a bit confused by this.  How is this ABI-breaking?  If (1) I'm right, and it isn't ABI-breaking, and (2) we're not going to have a custom macro, then I think this should just be behind NDEBUG.

ABI_BREAKING_CHECKS is meant to be for NDEBUG-like checks that actually break the ABI.  One could, in theory, build Clang with assertions, build LLVM without assertions, and still a functional compiler by overriding one of the defaults for ABI_BREAKING_CHECKS.  I don't see how that applies to these checks.


Repository:
  rL LLVM

https://reviews.llvm.org/D39245