[PATCH] D146727: [clangd] Use expansion location for missing include diagnostics.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:382
 
-        auto &Tokens = AST.getTokens();
-        auto SpelledForExpanded =
-            Tokens.spelledForExpanded(Tokens.expandedTokens(Ref.RefLocation));
-        if (!SpelledForExpanded)
+        auto ExpansionLoc = SM.getExpansionLoc(Ref.RefLocation);
+        const auto *Token = AST.getTokens().spelledTokenAt(ExpansionLoc);
----------------
kadircet wrote:
> can you actually get the spelling location first and use expansion location only when it isn't inside the main file?
> that way we get to preserve the behaviour for macro arguments, which warrants a test like:
> ```
> #define FOO(X) const X x
> 
> FOO([[Foo]]); // we should have a missing include for symbol Foo here
> ```
> 
> and can we have a comment about reasons and implications, e.g. something like:
> ```
> Spelling locations can point into preamble section at times. We don't want to emit diagnostics there as the offsets might be off when preamble is stale.
> In such cases we fallback to expansion locations, as they're always in the main file. This means we fail to attach diagnostics to spelling of symbol names inside macro bodies.
> // FIXME: Use presumed locations to make sure we can correctly generate diagnostics even in such cases.
> ```
Sam suggested a magic solution for this, which is `SM.getFileLoc(Ref.RefLocation)`. Its documentation says:

```
/// Given \p Loc, if it is a macro location return the expansion
/// location or the spelling location, depending on if it comes from a
/// macro argument or not.
```

Sounds like what we need, right?


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:36
     walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
-      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)) &&
+          SM.getDecomposedLoc(SM.getSpellingLoc(Loc)).first !=
----------------
kadircet wrote:
> you can directly use expansion location instead of checking both and mentioning preamble file id here, eg: `!SM.isWrittenInMainFile(SM.getExpansionLoc(Loc))`
This actually leads to both `BAR` and `Foo` in `#define BAR(x) Foo *x` in `foo.h` being recognized as missing and attaches 2 diagnostics to `[[BAR]](b)` in the main file. AFAIU, this is exactly what you're asking me to prevent in the previous comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146727