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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:281
+  std::vector<Expansion>
+  expansionsAffecting(llvm::ArrayRef<syntax::Token> Spelled) const;
 
----------------
kadircet wrote:
> this sounds more like `expansionsAffected` rather than affecting ? maybe my mental model requires correction, but it feels like spelled tokens are not affected by expansions, it is the other way around.
> 
> maybe even `expansionsSpawned` or `triggered`?
> 
> this is definitely non-blocking though, i am just bikesheding, comment is explanatory enough in any case.
Either mental model works I think. Mine is that expansions are rewrite rules - you start with a stream of tokens, and then each expansion mutates it, and you end up with different ones.

Renamed to `expansionsOverlapping` which seems more neutral and consistent with expansionStartingAt


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:431
+std::vector<TokenBuffer::Expansion>
+TokenBuffer::expansionsAffecting(llvm::ArrayRef<syntax::Token> Spelled) const {
+  if (Spelled.empty())
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84009