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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to