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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/trunk/CMakeLists.txt:400
+if( LLVM_ENABLE_ASSERTIONS )
+  set(LLVM_ENABLE_DUMP ON)
+endif()
----------------
hintonda wrote:
> MatzeB wrote:
> > MatzeB wrote:
> > > mehdi_amini wrote:
> > > > MatzeB wrote:
> > > > > hintonda wrote:
> > > > > > hintonda wrote:
> > > > > > > MatzeB wrote:
> > > > > > > > MatzeB wrote:
> > > > > > > > > hintonda wrote:
> > > > > > > > > > Just noticed that this doesn't actually update the cache.  However, that doesn't matter in this case since the updated variable is what is actually used.  
> > > > > > > > > > 
> > > > > > > > > > I'll fix this shortly so it won't confuse anyone looking at the cache.
> > > > > > > > > Yep it does not update the cache and that is a good thing in this situation IMO... I'd rather not see cmake change setting B just because I switched setting A...
> > > > > > > > Or put another way: That's why I used the "Enable Dump function **in release builds**" description to indicate the flag has no effect in debug builds because the feature is enabled anyway.
> > > > > > > > Yep it does not update the cache and that is a good thing in this situation IMO... I'd rather not see cmake change setting B just because I switched setting A...
> > > > > > > 
> > > > > > > We can do this by adding an additional variable, i.e., cache the passed variable, but determine LLVM_ENABLE_DUMP on the fly -- using the same name for both can be confusing.
> > > > > > > Or put another way: That's why I used the "Enable Dump function in release builds" description to indicate the flag has no effect in debug builds because the feature is enabled anyway.
> > > > > > 
> > > > > > That's a good point.  Once we remove the test for !defined(NDEBUG), LLVM_ENABLE_DUMP will be used for both, so we'll need to change that.
> > > > > Sure, any good name suggestions? I'll throw LLVM_FORCE_ENABLE_DUMP into the bikeshed discussion.
> > > > Do we need this?
> > > > What about changing is in the `config.h`:
> > > > 
> > > > ```
> > > > /* Define if LLVM_ENABLE_DUMP is enabled or not DNDEBUG */
> > > > #cmakedefine LLVM_ENABLE_DUMP
> > > > #ifndef NDEBUG
> > > > #ifndef LLVM_ENABLE_DUMP
> > > > #define LLVM_ENABLE_DUMP
> > > > #endif
> > > > #endif
> > > > ```
> > > > 
> > > > Wouldn't this cover all the uses?
> > > > 
> > > This solution gets us back into the trouble of picking up the users NDEBUG setting in the header and not necessarily the one used when compiling llvm.
> > Then again this is hardly the first instance of this problem in LLVM and we could start fixing it by adding something like LLVM_ENABLE_ASSERTION and use that instead of NDEBUG in our headers.
> After doing a mass update of llvm, and discovering additional issues with NDEBUG, I think it would be better to replace all instances of NDEBUG with the appropriate ${PROJECT}_NDEBUG, and set ${PROJECT})_NDEBUG via cmake (in either config.h or llvm_config.h).
> 
> That will break the dependency on NDEBUG, which we don't really control, and allow us to set it on a project basis.
> 
> I'm working on a POC patch right now.
> This solution gets us back into the trouble of picking up the users NDEBUG setting in the header and not necessarily the one used when compiling llvm.

Ah good point!


Repository:
  rL LLVM

https://reviews.llvm.org/D38306