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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

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"`?

> 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