[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 8 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:280
                                     const PreambleData &Baseline) {
+  assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
   // First scan the include directives in Baseline and Modified. These will be
----------------
sammccall wrote:
> BTW do we want to check isPreambleCompatible and bail out early if it is, or is that the caller's job?
I am planning to leave it to the caller. Eventually this PreamblePatch should be generated by TUScheduler, in which we can check for staleness and issue either an unmodified patch or create a patch and use it until new preamble is ready or patch is invalidated.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:332
+  for (auto &Inc : *ModifiedIncludes) {
+    auto It = ExistingIncludes.find({Inc.Directive, Inc.Written});
+    // Include already present in the baseline preamble. Set resolved path and
----------------
sammccall wrote:
> This explicitly looks at the existing includes based on spelling, and not whether the include was resolved during scanning or not.
> 
> One implication is that if you insert an #include that was already transitively included (which IIUC will hit the preamble VFS cache and thus be resolved) then we're not going to take advantage of this to record its resolution now.
> 
> Instead we're going to parse it along with the mainfile, and... well, I *hope* everything works out:
>  - do we pay for the stat, or is the preamble's stat cache still in effect?
>  - if the file is include-guarded, we're not going to read or parse it I guess?
>  - if it's not include-guarded but #imported, then again we won't re-read or re-parse it because we use the same directive?
>  - if the file isn't include-guarded, I guess we must read it (because preamble doesn't contain the actual buffer) and then parse it, but such is life. 
> 
> This explicitly looks at the existing includes based on spelling, and not whether the include was resolved during scanning or not.

Right. Scanning won't resolve anything as we pass an empty FS to preprocessor.

> Instead we're going to parse it along with the mainfile, and... well, I *hope* everything works out:
> do we pay for the stat, or is the preamble's stat cache still in effect?

It depends on the user of PreamblePatch, while building an AST we build preprocessor with cached fs coming from preamble.

> if the file is include-guarded, we're not going to read or parse it I guess?

Yes that's the case. We'll only see the include directive.

> if it's not include-guarded but #imported, then again we won't re-read or re-parse it because we use the same directive?

Yes again.

> if the file isn't include-guarded, I guess we must read it (because preamble doesn't contain the actual buffer) and then parse it, but such is life.

Unfortunately yes again, but as you said i am not sure if that's a case we should try and recover from as i can only see two use cases:
- You include the header in a different preprocessor state and intentionally want it to parse again, hopefully we'll handle this after macro patching.
- You deleted the include from a transitively included file and inserted it back into this file again. Well, now the hell breaks loose until we build the preamble again. In the worst case AST will turn into a garbage and we won't be able to serve anything but code completions, which is the state of the world without preamble patching, we are just consuming more cpu now.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:340
     }
     // FIXME: Traverse once
     auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset);
----------------
sammccall wrote:
> nice to have a comment similar to the one on line 333 for this case
> ```
> // Include is new in the modified preamble.
> // Inject it into the patch and use #line to set the presumed location to where it is spelled
> ```
handled in D78743


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77644/new/

https://reviews.llvm.org/D77644