Jay Foad <jay.foad at gmail.com> writes:>> Committed a change for the cmake build that implements the option >> LLVM_ENABLE_ASSERTS. Defaults to ON if and only if CMAKE_BUILD_TYPE is >> Release. > > Thanks! I think it might be nice to make this a bit more consistent > with Makefile.config. I'd suggest... > > - use LLVM_DISABLE_ASSERTIONS instead of LLVM_ENABLE_ASSERTSOkay.> - have it default to OFF (so assertions are enabled) alwaysIMHO this is wrong: if the user asks for a release build, assertions should be turned off by default. The fact that the configure/make build does not follow this policy caused some surprise among the LLVM developers and users on the past. So it is the configure/make build the one that needs to be fixed :-)> - compile with -DNDEBUG if assertions are off --Good catch!> I don't think -UNDEBUG has any effect, because NDEBUG won't have been > defined in the first placeRight. For Release builds, cmake automatically defines NDEBUG. -- Óscar
>> Thanks! I think it might be nice to make this a bit more consistent >> with Makefile.config. I'd suggest... >> >> - use LLVM_DISABLE_ASSERTIONS instead of LLVM_ENABLE_ASSERTS > > Okay.I meant ...DISABLE... instead of ...ENABLE...!>> I don't think -UNDEBUG has any effect, because NDEBUG won't have been >> defined in the first place > > Right. For Release builds, cmake automatically defines NDEBUG.OK, I didn't know that! Thanks, Jay.
Jay Foad <jay.foad at gmail.com> writes:>>> Thanks! I think it might be nice to make this a bit more consistent >>> with Makefile.config. I'd suggest... >>> >>> - use LLVM_DISABLE_ASSERTIONS instead of LLVM_ENABLE_ASSERTS >> >> Okay. > > I meant ...DISABLE... instead of ...ENABLE...!The `configure' script accepts --enable-x and --disable-x. CMake uses -DX=ON and -DX=OFF. For consistency, I chose to follow the naming scheme LLVM_ENABLE_X for all the switches supported by the cmake build. [snip] -- Óscar