<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jan 3, 2014 at 4:08 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 03/01/2014 19:41, Chandler Carruth wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
On Fri, Jan 3, 2014 at 2:03 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a> <mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>>> wrote:<br>
<br>
On Fri, Jan 3, 2014 at 10:15 AM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a><br></div><div class="im">
<mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>>> wrote:<br>
<br>
Ok, sounds like it's not clear if folks want to have dump<br>
methods behind #ifdef !NDEBUG. Maybe we don't have to resolve<br>
that part in this thread<br>
<br>
Is there any opposition to replacing the attributes on just<br>
the dump methods with LLVM_DUMP_METHOD? That's a subset of<br>
Chandler's change and I think also what Alp's diff is.<br>
<br>
…which is effectively Alp's patch I suppose. So maybe we could<br>
land that for now and then discuss whether to put dump() methods<br>
in !NDEBUG at some other point?<br>
<br>
<br>
I'd like to actually see the patch rather than talk in the hypothetical.<br>
</div></blockquote>
<br>
The patch was in the original mail you replied to.</blockquote><div><br></div><div>Sorry, totally missed the attachments when ctatching up on this thread. Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Maybe I misunderstood, but I thought it also moved the attribute to the definition? I don't understand why that's relevant yet (which is why maybe I've misunderstood what was happening).<br>
</blockquote>
<br></div>
Explained in the original mail.</blockquote><div><br></div><div>The first time I read it I got the impression there was some other bug besides the general desire to avoid NDEBUG in headers. Re-reading it, it seems that was all? Have I understood it correctly this time? <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br></div><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
My reading of the spec for the attribute seems to indicate that there is no difference....<br>
<br>
</blockquote>
<br></div>
If there's no spec difference between the declaration and definition forms, that's a good reason to avoid the form that's problematic and go with the simple patch in the original mail.</blockquote><div><br></div>
<div>Again, I got the impression there was something more than moving NDEBUG references out of headers, but it seems that impression was wrong.</div><div><br></div><div><br></div><div>Anyways, I don't actually agree with the conclusion of the NDEBUG-in-headers discussion, but we can continue that discussion on its original thread. I've posted the primary reason why I don't see how to do this across the entire LLVM project in the thread.</div>
<div><br></div><div>My real motivation here is simple:</div><div>- I really like the helper functions used exclusively to support "dump" or "assert" (things we don't expect to generally ship in the released compiler) to be clearly separated from other functions. Even better if I get a compiler error when I misuse one from normal code.</div>
<div>- I don't see any good way to do this if the only marking of dump methods are conditional "used" attributes. We don't want to similarly mark the helper functions (no need to call them from the debugger) and even if we did it wouldn't get us warnings when we mix them up.</div>
<div>- If we put guards around the very declaration of these methods, then we'll get errors when we call helpers from non-assert contexts, and we'll get warnings when we fail to appropriately guard a method that is only used from assert contexts.</div>
<div>- But that runs afoul of the desire to have NDEBUG-free headers. I disagree with that general direction, but lets discuss that on its thread. (Note, I don't think your current patches meaningfully harm the NDEBUG issue -- they are strictly ABI neutral.)</div>
<div><br></div><div>Regarding your existing patch as an incremental step, I have some comments:</div><div><br></div><div>- I suspect these should be controlled by !NDEBUG || LLVM_ENABLE_DUMP</div><div>- I don't know that "DEBUG" is the right word here, because of the ability to control them with the LLVM_ENABLE_DUMP build configuration option</div>
<div>- You don't remove NDEBUG || LLVM_ENABLE_DUMP complexity from any of the dump methods in LLVM... or apply this attribute to any of the existing dump methods that are missing it currently. I'm not sure we should, because I like the compile-away removal mechanism more than the link-away as outlined above. =/</div>
<div>- Why is noinline kept in the !NDEBUG case? Is that just for ABI neutrality?</div><div>- Won't this cause warnings for unused private methods in !NDEBUG builds when the dump methods are not marked used? If we leave the methods in place, it would seem natural to mark them as unused in the !NDEBUG case to avoid this.</div>
</div></div></div>