[PATCH] D45815: [libclang] Add options to limit skipping of function bodies

Nikolai Kosjar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 24 07:03:02 PDT 2018


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





More information about the cfe-commits mailing list