[PATCH] D26718: [llvm] Iterate SmallPtrSet in reverse order to uncover non-determinism in codegen

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

I think we reached the point of `good enough`!
So LGTM.



================
Comment at: unittests/ADT/CMakeLists.txt:67
+if(DEFINED LLVM_ENABLE_ABI_BREAKING_CHECKS)
+  add_definitions(-DLLVM_ENABLE_ABI_BREAKING_CHECKS)
+  list(APPEND ADTSources ReverseIterationTest.cpp)
----------------
mgrang wrote:
> mehdi_amini wrote:
> > mgrang wrote:
> > > mehdi_amini wrote:
> > > > mgrang wrote:
> > > > > Without explicitly passing LLVM_ENABLE_ABI_BREAKING_CHECKS to the compilation line of ReverseIteration.cpp, it always complains about undefined variable "ReverseIterate".
> > > > That's not great, but I don't see a better way right now, can you at least document it in file here?
> > > Didn't realize we needed to include llvm/Config/abi-breaking.h in the header for LLVM_ABI_BREAKING_CHECKS to be used. After including this in the header we no longer need an explicit add_definitions.
> > Even if you build in Release mode?
> > My understanding was that the add_definitions was enabling the unittest all the time, regardless of the build configuration.
> No, this test will only be enabled when LLVM_ENABLE_ABI_BREAKING_CHECKS is defined.
> 
> However, conditionally including this test file only when LLVM_ENABLE_ABI_BREAKING_CHECKS is defined produces an error in Release mode: "unknown file ReverseIteration.cpp"
> So I guess we need to put the condition on the test instead of the file.
I was trying to make the test work in release mode, and expecting the `add_definitions(-DLLVM_ENABLE_ABI_BREAKING_CHECKS)` to enable it even in release mode. However that wouldn't work because the include of `abi-breaking.h` will override the command line definition. May not worth being fixed there.


https://reviews.llvm.org/D26718