[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#884703, @MatzeB wrote:

> I like this change.
>
> In https://reviews.llvm.org/D38406#884686, @hintonda wrote:
>
> > While `#ifdef LLVM_ENABLE_DUMP` is arguably better than `#ifndef NDEBUG`, it's still problematic if we extend this to declarations in headers.  Some headers that declare `void dump() const`, don't include any headers, so they won't `#ifdef` away the declaration which would have helped catch the problem at compile-time.
>
>
> To put it differently: Your concern is that someone uses `#ifdef LLVM_ENABLE_DUMP` but forgets the `#include "llvm/Config/llvm-config.h"`?


Yes, but as you mentioned below, it would be pretty rare.  The only reason I brought it up would be to reduce churn if we did decide to go that route.

>> To fix this, we could test for a value instead of just definition, i.e., `#if LLVM_ENABLE_DUMP`, or even `#if LLVM_ENABLE_DUMP > [some value]`.
> 
> I'm not convinced that helps all that much (not that it is a problem either though).
>  You would expect to see compile problems when implementing the dump() function if llvm-config.h wasn't present in the header. It's also customary to include "foo.h" as first thing in "foo.cpp" so we have at least 1 file where we only have a no other headers included before "foo.h".
>  So I'd lean towards staying with `#ifdef LLVM_ENABLE_DUMP` as that is a more common idiom which avoids the other failure case where someone does `#ifdef XXX` working anyway even though XXX was defined to 0.
> 
> (No strong opinion from my side though so if others disagree I can be convinced to use this style).




https://reviews.llvm.org/D38406