[clang-tools-extra] decdbc1 - [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2019-12-09T16:34:01+01:00
New Revision: decdbc1155f5120554269319b1c77675bac9151c

URL: https://github.com/llvm/llvm-project/commit/decdbc1155f5120554269319b1c77675bac9151c
DIFF: https://github.com/llvm/llvm-project/commit/decdbc1155f5120554269319b1c77675bac9151c.diff

LOG: [clangd] Use expansion location when the ref is inside macros.

Summary:
Previously, xrefs has inconsistent behavior when the reference is inside
macro body:
- AST-based xrefs (for main file) uses the expansion location;
- our index uses the spelling location;

This patch makes our index use file locations for references, which is
consistent with AST-based xrefs, and kythe as well.

After this patch, memory usage of static index on LLVM increases ~5%.

Reviewers: ilya-biryukov

Subscribers: merge_guards_bot, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

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

Added: 
    

Modified: 
    clang-tools-extra/clangd/index/SymbolCollector.cpp
    clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
    clang-tools-extra/clangd/unittests/XRefsTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 191cd68ccb29..6775ccc5bc16 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -276,10 +276,9 @@ bool SymbolCollector::handleDeclOccurence(
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto &SM = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
-      SM.getFileID(SpellingLoc) == SM.getMainFileID())
+      SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,9 +311,14 @@ bool SymbolCollector::handleDeclOccurence(
       !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
     return true;
   // Do not store references to main-file symbols.
+  // Unlike other fields, e.g. Symbols (which use spelling locations), we use
+  // 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) &&
-      (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-    DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+      (Opts.RefsInHeaders ||
+       SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+    DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;

diff  --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index abc7aa389bd5..b5876b154c7b 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -718,6 +718,29 @@ TEST_F(SymbolCollectorTest, NameReferences) {
                                   HaveRanges(Header.ranges()))));
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  // Refs collected from SymbolCollector behave in the same way as
+  // AST-based xrefs.
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+    #define TYPE(X) X
+    #define FOO Foo
+    #define CAT(X, Y) X##Y
+    class [[Foo]] {};
+    void test() {
+      TYPE([[Foo]]) foo;
+      [[FOO]] foo2;
+      TYPE(TYPE([[Foo]])) foo3;
+      [[CAT]](Fo, o) foo4;
+    }
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+                                  HaveRanges(Header.ranges()))));
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(

diff  --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index ee23522d109a..e009c1c3ab20 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -811,6 +811,19 @@ TEST(FindReferences, WithinAST) {
         } // namespace ns
         int main() { [[^ns]]::Foo foo; }
       )cpp",
+
+      R"cpp(// Macros
+        #define TYPE(X) X
+        #define FOO Foo
+        #define CAT(X, Y) X##Y
+        class [[Fo^o]] {};
+        void test() {
+          TYPE([[Foo]]) foo;
+          [[FOO]] foo2;
+          TYPE(TYPE([[Foo]])) foo3;
+          [[CAT]](Fo, o) foo4;
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);