[PATCH] D50801: [rename] Use isPointWithin when looking for a declaration at location

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.
arphaman added reviewers: jkorous, hokein, ioeric.
Herald added a subscriber: dexonsmith.

This patch is a followup to https://reviews.llvm.org/D50740 .

This patch fixes the issue where clang-refactor local-rename was unable to find a declaration in a header file if that header file was included more than once, and the location of the point of interest was in a different included instance of a file than then declaration itself. The use of `isPointWithin` ensures that we're checking if the point is in the source range regardless of the included file.


Repository:
  rC Clang

https://reviews.llvm.org/D50801

Files:
  lib/Tooling/Refactoring/Rename/USRFinder.cpp
  test/Refactor/LocalRename/Inputs/MultiHeader.h
  test/Refactor/LocalRename/IsPointWithinDifferentInclude.c


Index: test/Refactor/LocalRename/IsPointWithinDifferentInclude.c
===================================================================
--- /dev/null
+++ test/Refactor/LocalRename/IsPointWithinDifferentInclude.c
@@ -0,0 +1,16 @@
+// RUN: clang-refactor local-rename -selection=%S/Inputs/MultiHeader.h:5:9 -new-name=renamedField %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// The MultiHeader file included twice.
+// The first inclusion contains no declarations. However, the selection
+// "MultiHeader:5:9" is mapped to a location in this inclusion of the file.
+#include "MultiHeader.h"
+
+// The second inclusion contains the declarations. The range of the
+// declaration we would like is located in this inclusion.
+#define HAVE_DECLS
+#include "MultiHeader.h"
+
+// Ensure that we still find the declaration even though the location and the
+// declaration's range are located in two different inclusions.
+// CHECK:      struct Foo {
+// CHECK-NEXT:   int renamedField;
Index: test/Refactor/LocalRename/Inputs/MultiHeader.h
===================================================================
--- /dev/null
+++ test/Refactor/LocalRename/Inputs/MultiHeader.h
@@ -0,0 +1,9 @@
+#ifdef HAVE_DECLS
+
+struct MyStruct {
+  struct Foo {
+    int field;
+  } bar;
+};
+
+#endif
Index: lib/Tooling/Refactoring/Rename/USRFinder.cpp
===================================================================
--- lib/Tooling/Refactoring/Rename/USRFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRFinder.cpp
@@ -48,7 +48,8 @@
       SourceLocation Start = Range.getBegin();
       SourceLocation End = Range.getEnd();
       if (!Start.isValid() || !Start.isFileID() || !End.isValid() ||
-          !End.isFileID() || !isPointWithin(Start, End))
+          !End.isFileID() ||
+          !Context.getSourceManager().isPointWithin(Point, Start, End))
         return true;
     }
     Result = ND;
@@ -58,14 +59,6 @@
   const NamedDecl *getNamedDecl() const { return Result; }
 
 private:
-  // Determines if the Point is within Start and End.
-  bool isPointWithin(const SourceLocation Start, const SourceLocation End) {
-    // FIXME: Add tests for Point == End.
-    return Point == Start || Point == End ||
-           (Context.getSourceManager().isBeforeInTranslationUnit(Start,
-                                                                 Point) &&
-            Context.getSourceManager().isBeforeInTranslationUnit(Point, End));
-  }
 
   const NamedDecl *Result = nullptr;
   const SourceLocation Point; // The location to find the NamedDecl.
@@ -80,14 +73,15 @@
   NamedDeclOccurrenceFindingVisitor Visitor(Point, Context);
 
   // Try to be clever about pruning down the number of top-level declarations we
-  // see. If both start and end is either before or after the point we're
-  // looking for the point cannot be inside of this decl. Don't even look at it.
+  // see. If the range of the declaration doesn't contain the source location,
+  // then don't even look at it.
   for (auto *CurrDecl : Context.getTranslationUnitDecl()->decls()) {
-    SourceLocation StartLoc = CurrDecl->getBeginLoc();
-    SourceLocation EndLoc = CurrDecl->getEndLoc();
-    if (StartLoc.isValid() && EndLoc.isValid() &&
-        SM.isBeforeInTranslationUnit(StartLoc, Point) !=
-            SM.isBeforeInTranslationUnit(EndLoc, Point))
+    CharSourceRange DeclRange = Lexer::makeFileCharRange(
+        CharSourceRange::getTokenRange(CurrDecl->getSourceRange()),
+        Context.getSourceManager(), Context.getLangOpts());
+    // FIXME: Use early break if the decl is found.
+    if (DeclRange.isValid() &&
+        SM.isPointWithin(Point, DeclRange.getBegin(), DeclRange.getEnd()))
       Visitor.TraverseDecl(CurrDecl);
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50801.160899.patch
Type: text/x-patch
Size: 3711 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180815/968ed47e/attachment.bin>