sammccall added inline comments.
================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:115 + /// expansion. + llvm::Optional<FileRange> range(const SourceManager &SM) const; + ---------------- 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. 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
