VitaNuo added a comment.
thanks for the comments!
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:380
+ auto Loc = SM.getFileLoc(Ref.RefLocation);
+ auto Range = Lexer::makeFileCharRange(
+ CharSourceRange{SourceRange{Loc}, true}, SM, AST.getLangOpts());
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > I don't think you need to re-lex the token here, as `Loc` is already a
> > > non-macro location in main file, you can still use
> > > `Tokens.spelledTokenAt(Loc)`. Am i missing something?
> > It's for consistency with clangd diagnostic generation
> > [here](http://google3/third_party/llvm/llvm-project/clang-tools-extra/clangd/Diagnostics.cpp;l=114;rcl=512058308)
> > (according to Sam), but both should work.
> > Would you rather prefer `Tokens.spelledTokenAt(Loc)`?
> I'd rather go with `spelledTokenAt` here, diagnostics code deals with
> locations outside of the main file, hence it makes sense for that logic to
> re-run the lexer. but i feel like re-running lexer here will actually create
> more confusion, as we tend to re-use results in token buffers rather than
> using raw lexer in rest of the codebase, as it confused me above.
thank you!
================
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:
> VitaNuo wrote:
> > 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?
> Rather than doing spelling & falling back to expansion, just doing
> `getFileLoc` definitely makes sense.
> But what we actually want here is always map to spelling (e.g. we want `Foo`
> in macro body, rather than macro invocation to be highlighted).
> It's just we can't have it all the time because accessing offsets coming from
> stale preamble might result in crashes. Hence can you also add the comment i
> mentioned above so that we don't forget? maybe with a rewording like:
> ```
> // We actually always want to map usages to their spellings, but spelling
> locations can point into preamble section.
> // Using these offsets could lead into crashes in presence of stale
> preambles. Hence we use `getFileLoc` instead to make sure it always points
> into main file.
> // FIXME: Use presumed locations to map such usages back to patched locations
> safely.
> ```
thanks!
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
