[PATCH] D134618: [Syntax] Fix macro-arg handling in TokenBuffer::spelledForExpanded

[Kadir Cetinkaya via Phabricator via cfe-commits]

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