[PATCH] D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds.
Matthias Braun via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 28 10:13:20 PDT 2017
MatzeB added inline comments.
================
Comment at: llvm/trunk/CMakeLists.txt:400
+if( LLVM_ENABLE_ASSERTIONS )
+ set(LLVM_ENABLE_DUMP ON)
+endif()
----------------
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.
Repository:
rL LLVM
https://reviews.llvm.org/D38306
More information about the llvm-commits
mailing list