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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
