[PATCH] D38511: [cmake] Add LLVM_DEBUG variable which can be used to replace NDEBUG

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added reviewers: chandlerc, echristo, hfinkel.
MatzeB added a comment.

Thanks for working on this!

Will we only use LLVM_DEBUG in the public headers or also in the implementation files and private headers?

Apart from `NDEBUG` there's also the peculiar case of `assert()`s in the headers; though they are probably less of a problem as they do not change the datastructures.

There is a danger that we will regress given that we are forced to set NDEBUG as well to keep `assert()` working, so we wouldn't notice if someone incorrectly adds new assert() or `#ifdef NDEBUG` into the headers. I sometimes wonder if we shouldn't add some verification/linting scripts to LLVM that are then run for each commit by a buildbot machine. In this case for example we could search the public headers for NDEBUG and warn/bail out if we find them. We can have this discussion separate from this patch of course.



================
Comment at: CMakeLists.txt:393-402
+  option(LLVM_FORCE_ENABLE_DUMP "Enable dump functions in release builds" OFF)
 else()
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" ON)
+  option(LLVM_FORCE_ENABLE_DUMP "Enable dump functions in release builds" ON)
+  set(LLVM_DEBUG ON)
 endif()
 
----------------
I think we already have the `LLVM_ENABLE_ASSERTIONS` flag and should not introduce yet another one. We may or may not use `LLVM_ENABLE_DEBUG` instead of `LLVM_ENABLE_ASSERTIONS` for the preprocessor define.


================
Comment at: include/llvm/Config/llvm-config.h.cmake:17-18
 
+/* Define if CMAKE_BUILD_TYPE is Debug */
+#cmakedefine LLVM_DEBUG
+
----------------
I think `LLVM_ENABLE_DEBUG` would be more in line with other existing flags.


https://reviews.llvm.org/D38511