[PATCH] D84009: [Syntax] expose API for expansions overlapping a spelled token range.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, LGTM! (and loved the choice of `overlapping`)



================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:431
+std::vector<TokenBuffer::Expansion>
+TokenBuffer::expansionsAffecting(llvm::ArrayRef<syntax::Token> Spelled) const {
+  if (Spelled.empty())
----------------
sammccall wrote:
> kadircet wrote:
> > this might be inconistent with spelledForExpanded from time to time, e.g:
> > 
> > ```
> > #define FOO(X) 123
> > #define BAR
> > 
> > void foo() {
> >   FOO(BA^R);
> > }
> > ```
> > 
> > normally BAR has no expansions, but I think it will get merged into outer macro expansion e.g. `123` coming from `FOO(BAR)`. (whereas spelledForExpanded will treat `BAR` in isolation)
> > 
> > not sure an important limitation but something to keep in mind.
> I don't understand the consistency you're looking for - AFAICS these are different functions that do different things - spelledForExpanded is much higher level.
> 
> Is there something in the name or comment I can add/remove to clarify?
> 
> > normally BAR has no expansions
> 
> Tokens don't really "have expansions" - they're *part of* expansions. Generally expansionsOverlapping() will return the expansions themselves that are overlapping, while spelledForExpanded will include the expanded tokens if the whole expansion is in-range.
> 
> The former is a building-block (returning Expansions seems like a clear indicator of that) that will usually require some postfiltering, while the latter is fairly directly usable.
> Tokens don't really "have expansions" - they're *part of* expansions. Generally expansionsOverlapping() will return the expansions themselves that are overlapping, while spelledForExpanded will include the expanded tokens if the whole expansion is in-range.

yeah `expansionsOverlapping() will return the expansions themselves that are overlapping` bit made my reasoning a little more solid, i am still thinking with the `spelled tokens might expand mindset` but I suppose `spelled tokens might be part of expansions` is a better reasoning.

I don't think any extra comments are necessary, at least in this layer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84009