[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri May 17 09:38:51 PDT 2019


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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59887





More information about the cfe-commits mailing list