ilya-biryukov added inline comments.
================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:115 + /// expansion. + llvm::Optional<FileRange> range(const SourceManager &SM) const; + ---------------- sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > I think this might need a more explicit name. It's reasonably obvious > > > that this needs to be optional for some cases (token pasting), but it's > > > not obvious at callsites that (or why) we don't use the spelling location > > > for macro-expanded tokens. > > > > > > One option would be just do that and add an expandedFromMacro() helper so > > > the no-macros version is easy to express too. > > > If we can't do that, maybe `directlySpelledRange` or something? > > As mentioned in the offline conversation, the idea is that mapping from a > > token inside a macro expansion to a spelled token should be handled by > > `TokenBuffer` and these two functions is really just a convenience helper > > to get to a range *after* the mapping. > > > > This change has been boiling for some time and I think the other bits of it > > seem to be non-controversial and agreed upon. > > Would it be ok if we land it with this function using a more concrete name > > (`directlySpelledRange` or something else) or move it into a separate > > change? > > > > There's a follow-up change that adds an 'expand macro' action to clangd, > > which both has a use-site for these method and provides a functional > > feature based on `TokenBuffer`. Iterating on the design with (even a > > single) use-case should be simpler. > If we do want to reflect expanded/spelled as types, this will rapidly get > harder to change. But we do need to make progress here. > > If passing spelled tokens is the intended/well-understood use, let's just > assert on that and not return Optional. Then I'm less worried about the name: > misuse will be reliably punished. Added an assertion. Not sure about the name, kept `range` for now for the lack of a better alternative. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59887/new/ https://reviews.llvm.org/D59887 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
