[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#945809, @bogner wrote:

> I don't really think we want to be messing with the CMAKE_CXX_FLAGS_{BUILD_TYPE} variables for this. All of this regex-replace stuff is very complicated and seems fragile.
>
> A couple of things need to happen:
>
> 1. LLVM_ENABLE_DUMP definitely needs to be defined in llvm-config.h, or we won't be able to safely use it in headers. Headers that use it should include llvm-config.h
> 2. While we probably don't want to set a cache variable's value based on the value of assertions, if the user explicitly specifies it we probably do want to trust them. I think having LLVM_ENABLE_DUMP be computed / non-cached and basing it off of LLVM_ENABLE_ASSERTIONS and LLVM_FORCE_ENABLE_DUMP is probably the clearest thing to do here, though that does hit the problem that people already having LLVM_ENABLE_DUMP in their caches. Maybe it would work to set the non-cached version based off of asserts iff the cached version is unset?


If this would work for XCode and MSVC generators -- the MSVC generator breakage Aaron reported was the main reason this was originally rolled back -- then I'd be all for it.

My solution would support buildbots, developer builds, and installed versions via llvm-config.  What you've proposed would only support ninja/make and installed versions.  Btw, I only use ninja, so I'd love to do what you propose -- why would anyone use anything else, right?  ;-)

However, I don't see a way to solve the multi-config issue without either using config specific variables, e.g., CMAKE_CXX_FLAGS_RELEASE (the solution I"m proposing), or config specific lvm-config.h files, e.g., builddir/Release/include/llvm-config.h and passing -I in dev builds -- installed versions would just install the correct llvm-config.h (which is what Mathias was alluding to).

Of course, I might be missing something, so if you've got a solution that will solve all of these problems, I'm all in.  I just don't see how hard coding it in a single llvm-config.h could work.


https://reviews.llvm.org/D38406