kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:82
+ // When First and Last are part of the *same macro arg* of a macro written
+ // in the main file, we may return only that part of the arg, i.e. their
+ // spelling range.
----------------
`main file` here and in rest of the comments feels a little confusing. maybe
rename `FID` to `MainFile` (which is still somewhat confusing) or call it
`SpelledFID` and also change the mentions of `main file` in comments to
`spelling file` (or similar). (assuming i am not misunderstanding something)
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:102
+ // Careful, given:
+ // #define HIDE ID(ID(a))
+ // ID(ID(HIDE))
----------------
s/a/A/
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:137
+ // ID2(prev, target) // selecting 'target' succeeds
+ // #define LARGE ID(prev, target)
+ // LARGE // selecting 'target' fails
----------------
nit: s/ID/ID2
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:475
+ // Mapping. We must use the general (SourceManager-based) algorithm.
+ if (FirstMapping && LastMapping && FirstMapping == LastMapping &&
+ SourceMgr->isMacroArgExpansion(First->location()) &&
----------------
nit: drop the check for `LastMapping`
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:487
+ First->location(), Last->location(), Prev, Next, FID, *SourceMgr);
+ if (Range.isInvalid())
+ return llvm::None;
----------------
can we also have a test like:
```
#define ID(X) X
#define BAR ID(1)
BAR
```
to make sure we can select the object-like macro when the whole contents are
matched (it's probably also worth mentioning in the public documentation)
================
Comment at: clang/unittests/Tooling/Syntax/TokensTest.cpp:760
+ EXPECT_THAT(Buffer.spelledForExpanded(findExpanded("y")),
+ ValueIs(SameRange(findSpelled("( y").drop_front())));
+
----------------
"y" is only mentioned once in the main-file no need for matching something wider
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134618/new/
https://reviews.llvm.org/D134618
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
