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