nik marked 3 inline comments as done. nik added a comment. In https://reviews.llvm.org/D45815#1076434, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D45815#1073581, @nik wrote: > > > Hmm, that indicates that template function bodies are actually not that > > dominant, which I also haven't thought. Now I'm doubting the correctness of > > my patch/tests. > > > The numbers are definitely interesting, we need to double-check if that it's > the case. But they definitely make a good point for including this option. > It seems Qt and LLVM are exactly the types of codebases where skipping the > templates doesn't bring much value, as most of the code is not templated. On > the other hand, skipping inline function might potentially be a win for those. > > I'm still tempted to say that we should either skip all bodies or none of > them to keep the code simpler, but I see why having errors from template > instantiations is preferable. OK, I've rechecked this change. I don't see any obvious mistake :) In my previous tests/timings I had some modifications to the clang command line applied (extra diagnostics enabled with -Weverything and maybe others, using -isystem instead of -I). I've reverted these and made some runs/timings with only "-Wall -Wextra" (https://dpaste.de/cZgZ/raw) - The difference between skip-all-in-preamble and skip-only-non-template-in-preamble becomes even smaller now for Qt Creator's texteditor.cpp. ================ Comment at: include/clang/Frontend/ASTUnit.h:679 + struct SkipFunctionBodiesOptions { + SkipFunctionBodiesOptions() {} + enum { None, MainFileAndPreamble, Preamble } Scope = None; ---------------- ilya-biryukov wrote: > Default ctor will be generated automatically. Maybe remove explicit > definition? The explicit definition is needed to workaround https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328 ("SkipFunctionBodiesOptions()" is used as default value in some function signatures) ================ Comment at: include/clang/Frontend/ASTUnit.h:679 + struct SkipFunctionBodiesOptions { + SkipFunctionBodiesOptions() {} + enum { None, MainFileAndPreamble, Preamble } Scope = None; ---------------- yvvan wrote: > nik wrote: > > ilya-biryukov wrote: > > > Default ctor will be generated automatically. Maybe remove explicit > > > definition? > > The explicit definition is needed to workaround > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328 > > ("SkipFunctionBodiesOptions()" is used as default value in some function > > signatures) > or replace by constructor with parameters That would not really help in this case. ================ Comment at: include/clang/Frontend/FrontendOptions.h:302 + SkipFunctionBodiesKind SkipFunctionBodies; + ---------------- ilya-biryukov wrote: > Maybe add a comment to match the code style of other options? No comment here is in line with "CodeCompleteOptions CodeCompleteOpts" below. That one has also its comments at definition. Also, I can't come up with a comment that adds more value than the name itself and that does not duplicate the comment from SkipFunctionBodies.h. ================ Comment at: lib/Frontend/ASTUnit.cpp:1662 + { + SkipFunctionBodiesModifierRAII m(Invocation->getFrontendOpts(), + SkipFunctionBodiesOpts); ---------------- ilya-biryukov wrote: > Maybe use LLVM's `make_scope_exit` from `ADT/ScopeExit.h` instead of creating > a separate RAII class? > Or even directy set/restore the value of the flag right in the function. > Given that LLVM does not use exceptions, RAII class does not seem to buy much > in terms of correctness and doesn't really make the code easier to read, IMO. > > But up to you. I've added the RAII class solely to minimize duplication. Now I'm just passing the flags to getMainBufferWithPrecompiledPreamble() and set/reset there directly. ================ Comment at: lib/Parse/Parser.cpp:1107 + bool SkipFunctionBodyRequested = false; + switch (SkipFunctionBodies) { ---------------- ilya-biryukov wrote: > `Requested` in this name is a bit confusing, e.g. it's not clear who > requested it. > Maybe rename to `TrySkipFunctionBody` or something similar? TrySkipFunctionBody somewhat collides with the function trySkippingFunctionBody(). I'm using "ShouldSkipFunctionBody" now. Repository: rC Clang https://reviews.llvm.org/D45815 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
