[PATCH] D84513: [clangd] Collect references for externally visible main-file symbols

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

Without this patch clangd does not collect references for main-file symbols if there is no public declaration in preamble.
Example:
`test1.c`

  void f1() {}

`test2.c`

  extern void f1();
  void f2() {
    f^1();
  }

`Find all references` does not show definition of f1() in the result, but GTD works OK.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84513

Files:
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -624,11 +624,13 @@
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "NS").ID, _))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "MACRO").ID,
                                   HaveRanges(Main.ranges("macro")))));
-  // Symbols *only* in the main file (a, b, c, FUNC) had no refs collected.
+  // Symbols *only* in the main file:
+  // - (a, b) externally visible and should have refs.
+  // - (c, FUNC) externally invisible and had no refs collected.
   auto MainSymbols =
       TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "a").ID, _))));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "b").ID, _))));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "a").ID, _)));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "b").ID, _)));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _))));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "FUNC").ID, _))));
 }
@@ -816,11 +818,15 @@
     $Foo[[Foo]] fo;
   }
   )");
-  // The main file is normal .cpp file, we shouldn't collect any refs of symbols
-  // which are not declared in the preamble.
+  // The main file is normal .cpp file, we should collect the refs
+  // for externally visible symbols.
   TestFileName = testPath("foo.cpp");
   runSymbolCollector("", Header.code());
-  EXPECT_THAT(Refs, UnorderedElementsAre());
+  EXPECT_THAT(Refs,
+              UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
+                                        HaveRanges(Header.ranges("Foo"))),
+                                   Pair(findSymbol(Symbols, "Func").ID,
+                                        HaveRanges(Header.ranges("Func")))));
 
   // Run the .h file as main file, we should collect the refs.
   TestFileName = testPath("foo.h");
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -171,7 +171,7 @@
       #endif
       )cpp";
   FS.Files[testPath("root/A.cc")] =
-      "#include \"A.h\"\nvoid g() { (void)common; }";
+      "#include \"A.h\"\nstatic void g() { (void)common; }";
   FS.Files[testPath("root/B.cc")] =
       R"cpp(
       #define A 0
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -314,7 +314,8 @@
   // file locations for references (as it aligns the behavior of clangd's
   // AST-based xref).
   // FIXME: we should try to use the file locations for other fields.
-  if (CollectRef && !IsMainFileOnly && !isa<NamespaceDecl>(ND) &&
+  if (CollectRef && (!IsMainFileOnly || ND->isExternallyVisible()) &&
+      !isa<NamespaceDecl>(ND) &&
       (Opts.RefsInHeaders ||
        SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
     DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
Index: clang-tools-extra/clangd/index/Serialization.cpp
===================================================================
--- clang-tools-extra/clangd/index/Serialization.cpp
+++ clang-tools-extra/clangd/index/Serialization.cpp
@@ -419,7 +419,7 @@
 // The current versioning scheme is simple - non-current versions are rejected.
 // If you make a breaking change, bump this version number to invalidate stored
 // data. Later we may want to support some backward compatibility.
-constexpr static uint32_t Version = 13;
+constexpr static uint32_t Version = 14;
 
 llvm::Expected<IndexFileIn> readRIFF(llvm::StringRef Data) {
   auto RIFF = riff::readFile(Data);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84513.280405.patch
Type: text/x-patch
Size: 4098 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200724/1f348f66/attachment.bin>