<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jan 3, 2014 at 12:43 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Jan 3, 2014, at 2:08 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>

<br>
><br>
> That said, I dislike this approach. I prefer actually *removing* the functions in non-asserts builds because then we will get early warnings if we miss some, and we don't have to rely on linker dead stripping to remove all of them.<br>

<br>
</div>The benefit of relying on the linker is that a project can easily use any particular dump/print function from the llvm/clang libraries that it sees fit (e.g. for logging purposes) and since the linker has the ultimate knowledge of what gets used or not, it "just works" without unnecessary code-bloat.<br>

For example, a project may just want to use "Decl::dump(raw_ostream &OS)" for logging; with your scheme it'd need to configure & build specially for this and accept the code bloat from _all_ dump functions across llvm/clang, just to use this one function.<br>

<br>
But we don't need to hypothesize, we can see a similar example in clang with '-ast-dump'.<br>
Do you really want to disable this option on release builds ? I would much prefer not, I've used it frequently enough and I'd like to continue to be able to use it from our release builds.<br></blockquote><div><br>
</div><div>Sorry, terminology is getting a bit hard here. I wasn't at all referring to the functionality of -ast-dump, or methods such as the one you mention which might legitimately be part of the C++ API that a particular type intends to expose. The AST is a great example here -- clearly it should provide ways for library users to print it out in the same nice textual form as used by -ast-dump.</div>
<div><br></div><div>I'm referring to the more narrow use of the term "dump method" which is specifically a method that accepts no arguments and prints as much state to stderr as might conceivably be useful from inside gdb. Decl::dump() is just such a function, but Decl::dump(raw_ostream &OS) is different. The somewhat consistent pattern in LLVM is to name the latter functions 'print' to avoid confusion, but that's neither here nor there.</div>
<div><br></div><div>My stance is essentially that the things which are only used either a) in GDB or b) inside of !NDEBUG regions (be they assert()s or the LLVM DEBUG macro) are the things which should be compiled away when NDEBUG (or the opt-in macro that several folks asked for in LLVM-land to get dump methods in no-asserts builds) and should be marked as noinline and used to make them easy to reach via the debugger.</div>
<div><br></div><div>When these routines can be implemented using the existing useful APIs like in the case of Decl, awesome. In other cases, there are no real users of the printing infrastructure and so all of it can be hidden in this way.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
><br>
> It also ensures that we don't grow accidental dependencies on functions that were only ever intended to be used inside of assert() and in dump() methods. Historically, this has happened quite a few times, and has taken someone some time to track down a compile time performance regression and separate out the functionality.<br>

<br>
</div>Could you elaborate with a specific example ? Was someone ever printing something by accident ?<br>
I think this could apply to invariant-checking functions, but for dump/print functionality I honestly don't think that there is much "danger" in practice.<br></blockquote><div><br></div><div>It was a traversal function. Specifically, it looked like a nice constant time query function, but actually did linear work under the hood. That was totally fine (and better than the complexity of other alternatives) when it was used as a helper to implement dump functionality. But it got re-used a few times, and ended up inside of an existing linear algorithm and caused us to have N^2 explosions every now and then.</div>
<div><br></div><div>Still, not the end of the world. It required a bug, some test cases, and some easy time with the profiler. But when it was suggested to hide all of the helper functions the same way as the dump methods in LLVM, making these compile errors was a really nice result.</div>
</div></div></div>