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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline 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);
----------------
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.
```


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:236
+  TU.AdditionalFiles["foo.h"] = guard(R"cpp(
+    #define FOO 1
+    struct Foo{}; 
----------------
can you also put a `#define BAR(x) Foo *x` here and a usage like `BAR(b);` inside the main file and check that we only get include for symbol for `BAR` inside the main file at that location?


================
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 !=
----------------
you can directly use expansion location instead of checking both and mentioning preamble file id here, eg: `!SM.isWrittenInMainFile(SM.getExpansionLoc(Loc))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146727