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