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

Wed Oct 25 22:50:31 PDT 2017

mgrang 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.
>  Is it possible to fixing the failures before enabling the switch? (or, in the same commit?) That should avoid bot breakage [I understand it's chasing a moving target, as we did with EXPENSIVE_CHECK, but it turned out to be worth in the long run fixing all the tests and turning the switch on instead of introducing new macros, cc: @RKSimon ]

Thanks Davide. So the consensus seems to be that ABI_BREAKING_CHECKS would be a better option to base this patch on. I have updated my patch accordingly.
We would need to fix the existing failures before this can be merged. I will solicit help from the code owners/developers of the breaking tests to get them fixed. Once those are fixed I would push patches to change std::sort to llvm::sort for llvm/clang/polly/compiler-rt.

Repository:
  rL LLVM

https://reviews.llvm.org/D39245