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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
