[PATCH] D31866: Keep NDEBUG out of header

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: include/llvm/MC/MCSchedule.h:29
   const char *Name;
-#endif
   unsigned NumUnits; // Number of resource of this kind
----------------
MatzeB wrote:
> jaykang10 wrote:
> > mehdi_amini wrote:
> > > I don't understand this change.
> > > We don't want to increase the runtime memory consumption when not needed. This should still be compiled out in release build.
> > > 
> > Sorry Mehdi. I know what you mean. In order to remove the "#ifndef NDEBUG" on dumpuses, we also need to remove it here. This patch's goal is to build llvm with cmake option -DCMAKE_CXX_FLAGS:STRING="-DLLVM_ENABLE_DUMP" -DCMAKE_BUILD_TYPE=RelWithDebInfo. First time, I suggested to add "|| defined(LLVM_ENABLE_DUMP)" with "#ifndef NDEBUG" to achieve the goal but I understand what Matthias want. I am not sure how I can satisfy you guys. If possible, can you suggest idea please? I will be very happy with it. 
> This is a case where the dumping code seems to be dependent on !NDEBUG code. You could solve this by tweaking the SchedBoundary::dumpScheduledState() implementation instead. (Like for example not calling getResourceName() with NDEBUG enabled).
I appreciate you guys comments. This issue is related to http://bugs.llvm.org/show_bug.cgi?id=32283. I think Chris will manage it overall. If possible, I would like to close this review.


https://reviews.llvm.org/D31866