[PATCH] D89468: [libTooling] Change `after` range-selector to operate only on source ranges

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: tdl-g.
Herald added a project: clang.
ymandel requested review of this revision.

Currently, `after` fails when applied to locations in macro arguments.  This
change projects the subrange into a file source range and then applies `after`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89468

Files:
  clang/lib/Tooling/Transformer/RangeSelector.cpp
  clang/unittests/Tooling/RangeSelectorTest.cpp


Index: clang/unittests/Tooling/RangeSelectorTest.cpp
===================================================================
--- clang/unittests/Tooling/RangeSelectorTest.cpp
+++ clang/unittests/Tooling/RangeSelectorTest.cpp
@@ -193,6 +193,51 @@
                        HasValue(EqualsCharSourceRange(ExpectedAfter)));
 }
 
+static void testAfterMacroArg(StringRef Code) {
+  const std::string Ref = "ref";
+  TestMatch Match =
+      matchCode(Code, declRefExpr(to(namedDecl(hasName("y")))).bind(Ref));
+  const auto *E = Match.Result.Nodes.getNodeAs<Expr>(Ref);
+  assert(E != nullptr);
+  // `E` is the variable `y`, and so is one character wide. So advance by one,
+  // bringing us to the next character.
+  SourceLocation Next =
+      Match.Result.SourceManager->getSpellingLoc(E->getBeginLoc())
+          .getLocWithOffset(1);
+  const auto ExpectedAfter = CharSourceRange::getCharRange(Next, Next);
+
+  EXPECT_THAT_EXPECTED(after(node(Ref))(Match.Result),
+                       HasValue(EqualsCharSourceRange(ExpectedAfter)));
+}
+
+// Test with a range that is the entire macro arg, but does not end the
+// expansion itself.
+TEST(RangeSelectorTest, AfterOpInMacroArg) {
+  StringRef Code = R"cc(
+#define ISNULL(x) x == nullptr
+    bool g() { int* y; return ISNULL(y); }
+  )cc";
+  testAfterMacroArg(Code);
+}
+
+// Test with a range that is the entire macro arg and ends the expansion itself.
+TEST(RangeSelectorTest, AfterOpInMacroArgEndsExpansion) {
+  StringRef Code = R"cc(
+#define ISNULL(x) nullptr == x
+    bool g() { int* y; return ISNULL(y); }
+  )cc";
+  testAfterMacroArg(Code);
+}
+
+TEST(RangeSelectorTest, AfterOpInPartOfMacroArg) {
+  StringRef Code = R"cc(
+#define ISNULL(x) x == nullptr
+    int* f(int*);
+    bool g() { int* y; return ISNULL(f(y)); }
+  )cc";
+  testAfterMacroArg(Code);
+}
+
 TEST(RangeSelectorTest, BetweenOp) {
   StringRef Code = R"cc(
     int f(int x, int y, int z) { return 3; }
Index: clang/lib/Tooling/Transformer/RangeSelector.cpp
===================================================================
--- clang/lib/Tooling/Transformer/RangeSelector.cpp
+++ clang/lib/Tooling/Transformer/RangeSelector.cpp
@@ -116,11 +116,24 @@
     Expected<CharSourceRange> SelectedRange = Selector(Result);
     if (!SelectedRange)
       return SelectedRange.takeError();
-    if (SelectedRange->isCharRange())
-      return CharSourceRange::getCharRange(SelectedRange->getEnd());
-    return CharSourceRange::getCharRange(Lexer::getLocForEndOfToken(
-        SelectedRange->getEnd(), 0, Result.Context->getSourceManager(),
-        Result.Context->getLangOpts()));
+    SourceLocation End = SelectedRange->getEnd();
+    if (SelectedRange->isTokenRange()) {
+      // We need to find the actual (exclusive) end location from which to
+      // create a new source range. However, that's not guaranteed to be valid,
+      // even if the token location itself is valid. So, we create a token range
+      // consisting only of the last token, then map that range back to the
+      // source file. If that succeeds, we have a valid location for the end of
+      // the generated range.
+      CharSourceRange Range = Lexer::makeFileCharRange(
+          CharSourceRange::getTokenRange(SelectedRange->getEnd()),
+          *Result.SourceManager, Result.Context->getLangOpts());
+      if (Range.isInvalid())
+        return invalidArgumentError(
+            "after: can't resolve sub-range to valid source range");
+      End = Range.getEnd();
+    }
+
+    return CharSourceRange::getCharRange(End);
   };
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89468.298378.patch
Type: text/x-patch
Size: 3560 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201015/3fcdaa04/attachment.bin>