[PATCH] D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks for fixing this :)



================
Comment at: CMakeLists.txt:397-401
+if( LLVM_ENABLE_ASSERTIONS )
+	option(LLVM_ENABLE_DUMP "Enable dump functions in release builds" ON)
+else()
+	option(LLVM_ENABLE_DUMP "Enable dump functions in release builds" OFF)
+endif()
----------------
MatzeB wrote:
> hintonda wrote:
> > MatzeB wrote:
> > > This is fragile, as for any subsequent `cmake` runs the value for LLVM_ENABLE_DUMP is already set in the cmake cache and won't change even if the user modifies `LLVM_ENABLE_ASSERTIONS`.
> > > 
> > > We rather want cmake code that sets the define if either this option or LLVM_ENABLE_ASSERTIONS is enabled.
> > So, are you saying it should always be set for +Asserts builds?
> > 
> > The email thread seemed to indicate they should vary independently.
> Yes.
> I cannot think of any reason why you would not want the dump functions in an asserts build. Also currently we use a `!defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` in the cpp files so we have to enable the dumpers in assert builds of llvm. Admittedly after this whole thread I wonder if we shouldn't simplify this to just `#ifdef LLVM_ENABLE_DUMP` but we better do that another time in a separate patch.
+1:

The code relate to dump should only be guarded by `#ifdef LLVM_ENABLE_DUMP `, and this should be on by default in Debug build, and then it is flexible enough and everyone is happy :)



Repository:
  rL LLVM

https://reviews.llvm.org/D38306