[clang-tools-extra] r373444 - [clangd] Bail out early if we are sure that the symbol is used outside of the file.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Oct  2 03:46:37 2019
New Revision: 373444

URL: http://llvm.org/viewvc/llvm-project?rev=373444&view=rev
Log:
[clangd] Bail out early if we are sure that the symbol is used outside of the 
file.

Summary:
This would reduce the false positive when the static index is in an
unavailable state, e.g. background index is not finished.

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, 
cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D68325

Modified:
    clang-tools-extra/trunk/clangd/refactor/Rename.cpp
    clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp

Modified: clang-tools-extra/trunk/clangd/refactor/Rename.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/Rename.cpp?rev=373444&r1=373443&r2=373444&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/refactor/Rename.cpp (original)
+++ clang-tools-extra/trunk/clangd/refactor/Rename.cpp Wed Oct  2 03:46:37 2019
@@ -83,9 +83,13 @@ llvm::Optional<ReasonToReject> renamable
   bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
   bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
 
+  if (!DeclaredInMainFile)
+    // We are sure the symbol is used externally, bail out early.
+    return UsedOutsideFile;
+
   // If the symbol is declared in the main file (which is not a header), we
   // rename it.
-  if (DeclaredInMainFile && !MainFileIsHeader)
+  if (!MainFileIsHeader)
     return None;
 
   // Below are cases where the symbol is declared in the header.

Modified: clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp?rev=373444&r1=373443&r2=373444&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp Wed Oct  2 
03:46:37 2019
@@ -95,7 +95,19 @@ TEST(RenameTest, Renameable) {
     const char *Code;
     const char* ErrorMessage; // null if no error
     bool IsHeaderFile;
+    const SymbolIndex *Index;
   };
+  TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
+  const char *CommonHeader = R"cpp(
+    class Outside {};
+    void foo();
+  )cpp";
+  OtherFile.HeaderCode = CommonHeader;
+  OtherFile.Filename = "other.cc";
+  // The index has a "Outside" reference and a "foo" reference.
+  auto OtherFileIndex = OtherFile.index();
+  const SymbolIndex *Index = OtherFileIndex.get();
+
   const bool HeaderFile = true;
   Case Cases[] = {
       {R"cpp(// allow -- function-local
@@ -103,59 +115,55 @@ TEST(RenameTest, Renameable) {
           [[Local]] = 2;
         }
       )cpp",
-       nullptr, HeaderFile},
+       nullptr, HeaderFile, Index},
 
       {R"cpp(// allow -- symbol is indexable and has no refs in index.
         void [[On^lyInThisFile]]();
       )cpp",
-       nullptr, HeaderFile},
+       nullptr, HeaderFile, Index},
 
       {R"cpp(// disallow -- symbol is indexable and has other refs in index.
         void f() {
           Out^side s;
         }
       )cpp",
-       "used outside main file", HeaderFile},
+       "used outside main file", HeaderFile, Index},
 
       {R"cpp(// disallow -- symbol is not indexable.
         namespace {
         class Unin^dexable {};
         }
       )cpp",
-       "not eligible for indexing", HeaderFile},
+       "not eligible for indexing", HeaderFile, Index},
 
       {R"cpp(// disallow -- namespace symbol isn't supported
         namespace fo^o {}
       )cpp",
-       "not a supported kind", HeaderFile},
+       "not a supported kind", HeaderFile, Index},
 
       {
           R"cpp(
          #define MACRO 1
          int s = MAC^RO;
        )cpp",
-          "not a supported kind", HeaderFile},
+          "not a supported kind", HeaderFile, Index},
 
       {
 
           R"cpp(
         struct X { X operator++(int) {} };
         void f(X x) {x+^+;})cpp",
-          "not a supported kind", HeaderFile},
+          "not a supported kind", HeaderFile, Index},
 
       {R"cpp(// foo is declared outside the file.
         void fo^o() {}
-      )cpp", "used outside main file", !HeaderFile/*cc file*/},
+      )cpp", "used outside main file", !HeaderFile /*cc file*/, Index},
+
+      {R"cpp(
+         // We should detect the symbol is used outside the file from the AST.
+         void fo^o() {})cpp",
+       "used outside main file", !HeaderFile, nullptr /*no index*/},
   };
-  const char *CommonHeader = R"cpp(
-    class Outside {};
-    void foo();
-  )cpp";
-  TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
-  OtherFile.HeaderCode = CommonHeader;
-  OtherFile.Filename = "other.cc";
-  // The index has a "Outside" reference and a "foo" reference.
-  auto OtherFileIndex = OtherFile.index();
 
   for (const auto& Case : Cases) {
     Annotations T(Case.Code);
@@ -170,7 +178,7 @@ TEST(RenameTest, Renameable) {
     auto AST = TU.build();
 
     auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
-                                    "dummyNewName", OtherFileIndex.get());
+                                    "dummyNewName", Case.Index);
     bool WantRename = true;
     if (T.ranges().empty())
       WantRename = false;


_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits