[PATCH] D38406: [dump] Remove NDEBUG from test to enable dump methods [NFC]

[Don Hinton via Phabricator via llvm-commits]

hintonda added a comment.

In https://reviews.llvm.org/D38406#947502, @MatzeB wrote:

> In https://reviews.llvm.org/D38406#947491, @hintonda wrote:
>
> > In https://reviews.llvm.org/D38406#947470, @MatzeB wrote:
> >
> > > Let's wait a bit to see if this is fine with people. It would probably be also nice to add the `set(LLVM_ENABLE_DUMP OFF CACHE BOOL "")` line to the varous caches in clang/cmake/caches that build llvm in release build types.
> >
> >
> > I don't mind changing the default, but the original default was OFF, and only flipped to ON if LLVM_ENABLE_ASSERTIONS was ON.  But that had no affect, since enabling assertions removed -DNDEBUG and added -UNDEBUG, so all the dump definitions were already enabled.  Setting LLVM_ENABLE_DUMP=ON only makes sense for non-debug builds where LLVM_ENABLE_ASSERTIONS=OFF.
> >
> > So, I think maintaining the default of OFF is the correct choice, but will set it to whatever you want.
>
>
> Wait, I'm getting confused with all those discussions going in circles. I assumed we would also change the code inside llvm to simply test for LLVM_ENABLE_DUMP and not for `!defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)`. But apparently we do not do that, in that case yes we can leave the default to "OFF" and the description to be "Enable dump functions even when assertions are disabled".


That sounds much better.  I'll make that change.


https://reviews.llvm.org/D38406