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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, mostly LG with a comment about some edge case. just wanna know what you think.



================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:281
+  std::vector<Expansion>
+  expansionsAffecting(llvm::ArrayRef<syntax::Token> Spelled) const;
 
----------------
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.


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


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:254
   // `Spelled` must be a subrange of `File.SpelledTokens`.
   assert(File.SpelledTokens.data() <= Spelled.data());
   assert(&Spelled.back() <=
----------------
these two are also asserted in `fileForSpelled`


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:257
          File.SpelledTokens.data() + File.SpelledTokens.size());
 #ifndef NDEBUG
   auto T1 = Spelled.back().location();
----------------
maybe move that into fileForSpelled too ? (to ensure `Spelled` is contained within the returned file)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84009