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

[Sam McCall via Phabricator via cfe-commits]

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