[PATCH] D38511: [cmake] Add LLVM_DEBUG variable which can be used to replace NDEBUG
Don Hinton via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 3 13:07:02 PDT 2017
hintonda added a comment.
In https://reviews.llvm.org/D38511#887415, @MatzeB wrote:
> Will we only use LLVM_DEBUG in the public headers or also in the implementation files and private headers?
I'd suggest outlawing NDEBUG, but initially the problem is usage of NDEBUG in public 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.
We could always add something like LLVM_ASSERT (or llvm_assert), if that becomes an issue.
> 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.
Okay, sounds good.
================
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()
----------------
MatzeB wrote:
> 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.
This change only adds LLVM_DEBUG.
The other changes you see are from D38406 which hasn't been approved yet, and just clean up setting LLVM_ENABLE_DUMP.
================
Comment at: include/llvm/Config/llvm-config.h.cmake:17-18
+/* Define if CMAKE_BUILD_TYPE is Debug */
+#cmakedefine LLVM_DEBUG
+
----------------
MatzeB wrote:
> I think `LLVM_ENABLE_DEBUG` would be more in line with other existing flags.
Will do...
https://reviews.llvm.org/D38511
More information about the llvm-commits
mailing list