Skip to content

Conversation

timon-ul
Copy link
Contributor

In case of the callHierarchy being invoked on a virtual function it would not display potential calls through the base function. This commit adds these potential calls and also gives the client the information to annotate them as such. Also adds a test for this.

In case of the callHierarchy being invoked on a virtual function it
would not display potential calls through the base function. This commit
adds these potential calls and also gives the client the information to
annotate them as such. Also adds a test for this.
@timon-ul
Copy link
Contributor Author

Pullrequest for clangd/clangd#1558

@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2025

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: None (timon-ul)

Changes

In case of the callHierarchy being invoked on a virtual function it would not display potential calls through the base function. This commit adds these potential calls and also gives the client the information to annotate them as such. Also adds a test for this.


Patch is 22.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/163024.diff

14 Files Affected:

  • (modified) clang-tools-extra/clangd/Protocol.h (+4)
  • (modified) clang-tools-extra/clangd/XRefs.cpp (+58-42)
  • (modified) clang-tools-extra/clangd/index/Index.cpp (+6)
  • (modified) clang-tools-extra/clangd/index/Index.h (+12)
  • (modified) clang-tools-extra/clangd/index/MemIndex.cpp (+21)
  • (modified) clang-tools-extra/clangd/index/MemIndex.h (+14-1)
  • (modified) clang-tools-extra/clangd/index/Merge.cpp (+24)
  • (modified) clang-tools-extra/clangd/index/Merge.h (+3)
  • (modified) clang-tools-extra/clangd/index/ProjectAware.cpp (+12)
  • (modified) clang-tools-extra/clangd/index/dex/Dex.cpp (+21)
  • (modified) clang-tools-extra/clangd/index/dex/Dex.h (+13-1)
  • (modified) clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp (+37)
  • (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+4)
  • (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+8)
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 3a6bf155ee153..0f51665d7b896 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1627,6 +1627,10 @@ struct CallHierarchyIncomingCall {
   /// The range at which the calls appear.
   /// This is relative to the caller denoted by `From`.
   std::vector<Range> fromRanges;
+
+  /// This caller might be a false positive.
+  /// We currently have no way of 100% confirming this.
+  bool mightNeverCall = false;
 };
 llvm::json::Value toJSON(const CallHierarchyIncomingCall &);
 
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 05e04ac161e54..677858bb8dcb3 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -21,6 +21,7 @@
 #include "clang-include-cleaner/Types.h"
 #include "index/Index.h"
 #include "index/Merge.h"
+#include "index/Ref.h"
 #include "index/Relation.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolID.h"
@@ -56,6 +57,7 @@
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallSet.h"
@@ -66,6 +68,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include <algorithm>
 #include <optional>
 #include <string>
 #include <vector>
@@ -2350,51 +2353,64 @@ incomingCalls(const CallHierarchyItem &Item, const SymbolIndex *Index) {
   // to an AST node isn't cheap, particularly when the declaration isn't
   // in the main file.
   // FIXME: Consider also using AST information when feasible.
-  RefsRequest Request;
-  Request.IDs.insert(*ID);
-  Request.WantContainer = true;
-  // We could restrict more specifically to calls by introducing a new RefKind,
-  // but non-call references (such as address-of-function) can still be
-  // interesting as they can indicate indirect calls.
-  Request.Filter = RefKind::Reference;
-  // Initially store the ranges in a map keyed by SymbolID of the caller.
-  // This allows us to group different calls with the same caller
-  // into the same CallHierarchyIncomingCall.
-  llvm::DenseMap<SymbolID, std::vector<Location>> CallsIn;
-  // We can populate the ranges based on a refs request only. As we do so, we
-  // also accumulate the container IDs into a lookup request.
-  LookupRequest ContainerLookup;
-  Index->refs(Request, [&](const Ref &R) {
-    auto Loc = indexToLSPLocation(R.Location, Item.uri.file());
-    if (!Loc) {
-      elog("incomingCalls failed to convert location: {0}", Loc.takeError());
-      return;
-    }
-    CallsIn[R.Container].push_back(*Loc);
+  auto QueryIndex = [&](llvm::DenseSet<SymbolID> IDs, bool MightNeverCall) {
+    RefsRequest Request;
+    Request.IDs = std::move(IDs);
+    Request.WantContainer = true;
+    // We could restrict more specifically to calls by introducing a new
+    // RefKind, but non-call references (such as address-of-function) can still
+    // be interesting as they can indicate indirect calls.
+    Request.Filter = RefKind::Reference;
+    // Initially store the ranges in a map keyed by SymbolID of the caller.
+    // This allows us to group different calls with the same caller
+    // into the same CallHierarchyIncomingCall.
+    llvm::DenseMap<SymbolID, std::vector<Location>> CallsIn;
+    // We can populate the ranges based on a refs request only. As we do so, we
+    // also accumulate the container IDs into a lookup request.
+    LookupRequest ContainerLookup;
+    Index->refs(Request, [&](const Ref &R) {
+      auto Loc = indexToLSPLocation(R.Location, Item.uri.file());
+      if (!Loc) {
+        elog("incomingCalls failed to convert location: {0}", Loc.takeError());
+        return;
+      }
+      CallsIn[R.Container].push_back(*Loc);
 
-    ContainerLookup.IDs.insert(R.Container);
-  });
-  // Perform the lookup request and combine its results with CallsIn to
-  // get complete CallHierarchyIncomingCall objects.
-  Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
-    auto It = CallsIn.find(Caller.ID);
-    assert(It != CallsIn.end());
-    if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
-      std::vector<Range> FromRanges;
-      for (const Location &L : It->second) {
-        if (L.uri != CHI->uri) {
-          // Call location not in same file as caller.
-          // This can happen in some edge cases. There's not much we can do,
-          // since the protocol only allows returning ranges interpreted as
-          // being in the caller's file.
-          continue;
+      ContainerLookup.IDs.insert(R.Container);
+    });
+    // Perform the lookup request and combine its results with CallsIn to
+    // get complete CallHierarchyIncomingCall objects.
+    Index->lookup(ContainerLookup, [&](const Symbol &Caller) {
+      auto It = CallsIn.find(Caller.ID);
+      assert(It != CallsIn.end());
+      if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file())) {
+        std::vector<Range> FromRanges;
+        for (const Location &L : It->second) {
+          if (L.uri != CHI->uri) {
+            // Call location not in same file as caller.
+            // This can happen in some edge cases. There's not much we can do,
+            // since the protocol only allows returning ranges interpreted as
+            // being in the caller's file.
+            continue;
+          }
+          FromRanges.push_back(L.range);
         }
-        FromRanges.push_back(L.range);
+        Results.push_back(CallHierarchyIncomingCall{
+            std::move(*CHI), std::move(FromRanges), MightNeverCall});
       }
-      Results.push_back(
-          CallHierarchyIncomingCall{std::move(*CHI), std::move(FromRanges)});
-    }
-  });
+    });
+  };
+  QueryIndex({ID.get()}, false);
+  // In the case of being a virtual function we also want to return
+  // potential calls through the base function.
+  if (Item.kind == SymbolKind::Method) {
+    llvm::DenseSet<SymbolID> IDs;
+    RelationsRequest Req{{ID.get()}, RelationKind::OverriddenBy};
+    Index->reverseRelations(Req, [&](const SymbolID &, const Symbol &Caller) {
+      IDs.insert(Caller.ID);
+    });
+    QueryIndex(std::move(IDs), true);
+  }
   // Sort results by name of container.
   llvm::sort(Results, [](const CallHierarchyIncomingCall &A,
                          const CallHierarchyIncomingCall &B) {
diff --git a/clang-tools-extra/clangd/index/Index.cpp b/clang-tools-extra/clangd/index/Index.cpp
index 86dc6ed763344..a2ec910606d92 100644
--- a/clang-tools-extra/clangd/index/Index.cpp
+++ b/clang-tools-extra/clangd/index/Index.cpp
@@ -77,6 +77,12 @@ void SwapIndex::relations(
   return snapshot()->relations(R, CB);
 }
 
+void SwapIndex::reverseRelations(
+    const RelationsRequest &R,
+    llvm::function_ref<void(const SymbolID &, const Symbol &)> CB) const {
+  return snapshot()->reverseRelations(R, CB);
+}
+
 llvm::unique_function<IndexContents(llvm::StringRef) const>
 SwapIndex::indexedFiles() const {
   // The index snapshot should outlive this method return value.
diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h
index a193b1a191216..3f2c9fca3cf52 100644
--- a/clang-tools-extra/clangd/index/Index.h
+++ b/clang-tools-extra/clangd/index/Index.h
@@ -181,6 +181,14 @@ class SymbolIndex {
       llvm::function_ref<void(const SymbolID &Subject, const Symbol &Object)>
           Callback) const = 0;
 
+  /// Finds all relations (O, P, S) stored in the index such that S is among
+  /// Req.Subjects and P is Req.Predicate, and invokes \p Callback for (S, O) in
+  /// each. Currently only allows the OverridenBy relation and only stored in Memory.
+  virtual void reverseRelations(
+      const RelationsRequest &Req,
+      llvm::function_ref<void(const SymbolID &Subject, const Symbol &Object)>
+          Callback) const = 0;
+
   /// Returns function which checks if the specified file was used to build this
   /// index or not. The function must only be called while the index is alive.
   using IndexedFiles =
@@ -214,6 +222,10 @@ class SwapIndex : public SymbolIndex {
                  llvm::function_ref<void(const SymbolID &, const Symbol &)>)
       const override;
 
+  void reverseRelations(const RelationsRequest &,
+                        llvm::function_ref<void (const SymbolID &, const Symbol &)>)
+      const override;
+
   llvm::unique_function<IndexContents(llvm::StringRef) const>
   indexedFiles() const override;
 
diff --git a/clang-tools-extra/clangd/index/MemIndex.cpp b/clang-tools-extra/clangd/index/MemIndex.cpp
index 9c9d3942bdee6..238f68dd37d19 100644
--- a/clang-tools-extra/clangd/index/MemIndex.cpp
+++ b/clang-tools-extra/clangd/index/MemIndex.cpp
@@ -125,6 +125,27 @@ void MemIndex::relations(
   }
 }
 
+void MemIndex::reverseRelations(
+    const RelationsRequest &Req,
+    llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
+  assert(Req.Predicate == RelationKind::OverriddenBy);
+  uint32_t Remaining = Req.Limit.value_or(std::numeric_limits<uint32_t>::max());
+  for (const SymbolID &Subject : Req.Subjects) {
+    LookupRequest LookupReq;
+    auto It = ReverseRelations.find(
+        std::make_pair(Subject, static_cast<uint8_t>(Req.Predicate)));
+    if (It != ReverseRelations.end()) {
+      for (const auto &Obj : It->second) {
+        if (Remaining > 0) {
+          --Remaining;
+          LookupReq.IDs.insert(Obj);
+        }
+      }
+    }
+    lookup(LookupReq, [&](const Symbol &Object) { Callback(Subject, Object); });
+  }
+}
+
 llvm::unique_function<IndexContents(llvm::StringRef) const>
 MemIndex::indexedFiles() const {
   return [this](llvm::StringRef FileURI) {
diff --git a/clang-tools-extra/clangd/index/MemIndex.h b/clang-tools-extra/clangd/index/MemIndex.h
index fb1052b0c7ca8..23cc8f030591c 100644
--- a/clang-tools-extra/clangd/index/MemIndex.h
+++ b/clang-tools-extra/clangd/index/MemIndex.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_MEMINDEX_H
 
 #include "index/Index.h"
+#include "index/Relation.h"
 #include "llvm/ADT/StringSet.h"
 #include <mutex>
 
@@ -27,10 +28,16 @@ class MemIndex : public SymbolIndex {
       Index[S.ID] = &S;
     for (const std::pair<SymbolID, llvm::ArrayRef<Ref>> &R : Refs)
       this->Refs.try_emplace(R.first, R.second.begin(), R.second.end());
-    for (const Relation &R : Relations)
+    for (const Relation &R : Relations) {
       this->Relations[std::make_pair(R.Subject,
                                      static_cast<uint8_t>(R.Predicate))]
           .push_back(R.Object);
+      if (R.Predicate == RelationKind::OverriddenBy) {
+        this->ReverseRelations[std::make_pair(R.Object,
+                                     static_cast<uint8_t>(R.Predicate))]
+          .push_back(R.Subject);
+      }
+    }
   }
   // Symbols are owned by BackingData, Index takes ownership.
   template <typename SymbolRange, typename RefRange, typename RelationRange,
@@ -80,6 +87,10 @@ class MemIndex : public SymbolIndex {
                  llvm::function_ref<void(const SymbolID &, const Symbol &)>
                      Callback) const override;
 
+  void reverseRelations(const RelationsRequest &Req,
+                 llvm::function_ref<void(const SymbolID &, const Symbol &)>
+                     Callback) const override;
+
   llvm::unique_function<IndexContents(llvm::StringRef) const>
   indexedFiles() const override;
 
@@ -94,6 +105,8 @@ class MemIndex : public SymbolIndex {
   static_assert(sizeof(RelationKind) == sizeof(uint8_t),
                 "RelationKind should be of same size as a uint8_t");
   llvm::DenseMap<std::pair<SymbolID, uint8_t>, std::vector<SymbolID>> Relations;
+  // Reverse relations, currently only for OverridenBy
+  llvm::DenseMap<std::pair<SymbolID, uint8_t>, std::vector<SymbolID>> ReverseRelations;
   // Set of files which were used during this index build.
   llvm::StringSet<> Files;
   // Contents of the index (symbols, references, etc.)
diff --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp
index aecca38a885b6..a7868b6f8688b 100644
--- a/clang-tools-extra/clangd/index/Merge.cpp
+++ b/clang-tools-extra/clangd/index/Merge.cpp
@@ -221,6 +221,30 @@ void MergedIndex::relations(
   });
 }
 
+void MergedIndex::reverseRelations(
+    const RelationsRequest &Req,
+    llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
+  uint32_t Remaining = Req.Limit.value_or(std::numeric_limits<uint32_t>::max());
+  // Return results from both indexes but avoid duplicates.
+  // We might return stale relations from the static index;
+  // we don't currently have a good way of identifying them.
+  llvm::DenseSet<std::pair<SymbolID, SymbolID>> SeenRelations;
+  Dynamic->reverseRelations(Req, [&](const SymbolID &Subject, const Symbol &Object) {
+    Callback(Subject, Object);
+    SeenRelations.insert(std::make_pair(Subject, Object.ID));
+    --Remaining;
+  });
+  if (Remaining == 0)
+    return;
+  Static->reverseRelations(Req, [&](const SymbolID &Subject, const Symbol &Object) {
+    if (Remaining > 0 &&
+        !SeenRelations.count(std::make_pair(Subject, Object.ID))) {
+      --Remaining;
+      Callback(Subject, Object);
+    }
+  });
+}
+
 // Returns true if \p L is (strictly) preferred to \p R (e.g. by file paths). If
 // neither is preferred, this returns false.
 static bool prefer(const SymbolLocation &L, const SymbolLocation &R) {
diff --git a/clang-tools-extra/clangd/index/Merge.h b/clang-tools-extra/clangd/index/Merge.h
index 7441be6e57e85..36c3567e7caec 100644
--- a/clang-tools-extra/clangd/index/Merge.h
+++ b/clang-tools-extra/clangd/index/Merge.h
@@ -44,6 +44,9 @@ class MergedIndex : public SymbolIndex {
   void relations(const RelationsRequest &,
                  llvm::function_ref<void(const SymbolID &, const Symbol &)>)
       const override;
+  void reverseRelations(const RelationsRequest &,
+                 llvm::function_ref<void(const SymbolID &, const Symbol &)>)
+      const override;
   llvm::unique_function<IndexContents(llvm::StringRef) const>
   indexedFiles() const override;
   size_t estimateMemoryUsage() const override {
diff --git a/clang-tools-extra/clangd/index/ProjectAware.cpp b/clang-tools-extra/clangd/index/ProjectAware.cpp
index 9836f0130362a..60d9710ab5640 100644
--- a/clang-tools-extra/clangd/index/ProjectAware.cpp
+++ b/clang-tools-extra/clangd/index/ProjectAware.cpp
@@ -51,6 +51,10 @@ class ProjectAwareIndex : public SymbolIndex {
                  llvm::function_ref<void(const SymbolID &, const Symbol &)>
                      Callback) const override;
 
+  void reverseRelations(const RelationsRequest &,
+                        llvm::function_ref<void (const SymbolID &, const Symbol &)>)
+      const override;
+
   llvm::unique_function<IndexContents(llvm::StringRef) const>
   indexedFiles() const override;
 
@@ -124,6 +128,14 @@ void ProjectAwareIndex::relations(
     return Idx->relations(Req, Callback);
 }
 
+void ProjectAwareIndex::reverseRelations(
+    const RelationsRequest &Req,
+    llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
+  trace::Span Tracer("ProjectAwareIndex::relations");
+  if (auto *Idx = getIndex())
+    return Idx->reverseRelations(Req, Callback);
+}
+
 llvm::unique_function<IndexContents(llvm::StringRef) const>
 ProjectAwareIndex::indexedFiles() const {
   trace::Span Tracer("ProjectAwareIndex::indexedFiles");
diff --git a/clang-tools-extra/clangd/index/dex/Dex.cpp b/clang-tools-extra/clangd/index/dex/Dex.cpp
index 575a96a112979..925e256cc5759 100644
--- a/clang-tools-extra/clangd/index/dex/Dex.cpp
+++ b/clang-tools-extra/clangd/index/dex/Dex.cpp
@@ -379,6 +379,27 @@ void Dex::relations(
   }
 }
 
+void Dex::reverseRelations(
+    const RelationsRequest &Req,
+    llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
+  assert(Req.Predicate == RelationKind::OverriddenBy);
+  uint32_t Remaining = Req.Limit.value_or(std::numeric_limits<uint32_t>::max());
+  for (const SymbolID &Subject : Req.Subjects) {
+    LookupRequest LookupReq;
+    auto It = ReverseRelations.find(
+        std::make_pair(Subject, static_cast<uint8_t>(Req.Predicate)));
+    if (It != ReverseRelations.end()) {
+      for (const auto &Obj : It->second) {
+        if (Remaining > 0) {
+          --Remaining;
+          LookupReq.IDs.insert(Obj);
+        }
+      }
+    }
+    lookup(LookupReq, [&](const Symbol &Object) { Callback(Subject, Object); });
+  }
+}
+
 llvm::unique_function<IndexContents(llvm::StringRef) const>
 Dex::indexedFiles() const {
   return [this](llvm::StringRef FileURI) {
diff --git a/clang-tools-extra/clangd/index/dex/Dex.h b/clang-tools-extra/clangd/index/dex/Dex.h
index 502f597d81ef0..274d2655cbebe 100644
--- a/clang-tools-extra/clangd/index/dex/Dex.h
+++ b/clang-tools-extra/clangd/index/dex/Dex.h
@@ -43,10 +43,16 @@ class Dex : public SymbolIndex {
       this->Symbols.push_back(&Sym);
     for (auto &&Ref : Refs)
       this->Refs.try_emplace(Ref.first, Ref.second);
-    for (auto &&Rel : Relations)
+    for (auto &&Rel : Relations) {
       this->Relations[std::make_pair(Rel.Subject,
                                      static_cast<uint8_t>(Rel.Predicate))]
           .push_back(Rel.Object);
+      if (Rel.Predicate == RelationKind::OverriddenBy) {
+        this->ReverseRelations[std::make_pair(Rel.Object,
+                                     static_cast<uint8_t>(Rel.Predicate))]
+          .push_back(Rel.Subject);
+      }
+    }
     buildIndex(SupportContainedRefs);
   }
   // Symbols and Refs are owned by BackingData, Index takes ownership.
@@ -96,6 +102,10 @@ class Dex : public SymbolIndex {
                  llvm::function_ref<void(const SymbolID &, const Symbol &)>
                      Callback) const override;
 
+  void reverseRelations(const RelationsRequest &,
+                        llvm::function_ref<void (const SymbolID &, const Symbol &)>)
+      const override;
+
   llvm::unique_function<IndexContents(llvm::StringRef) const>
   indexedFiles() const override;
 
@@ -142,6 +152,8 @@ class Dex : public SymbolIndex {
   static_assert(sizeof(RelationKind) == sizeof(uint8_t),
                 "RelationKind should be of same size as a uint8_t");
   llvm::DenseMap<std::pair<SymbolID, uint8_t>, std::vector<SymbolID>> Relations;
+  // Reverse relations, currently only for OverridenBy
+  llvm::DenseMap<std::pair<SymbolID, uint8_t>, std::vector<SymbolID>> ReverseRelations;
   std::shared_ptr<void> KeepAlive; // poor man's move-only std::any
   // Set of files which were used during this index build.
   llvm::StringSet<> Files;
diff --git a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
index 08cc80ff8981e..d906afe95703c 100644
--- a/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -162,6 +162,43 @@ TEST(CallHierarchy, IncomingOneFileObjC) {
   EXPECT_THAT(IncomingLevel4, IsEmpty());
 }
 
+TEST(CallHierarchy, IncomingIncludeOverrides) {
+  Annotations Source(R"cpp(
+    void call^ee() {}
+    struct Interface {
+      virtual void Func() = 0;
+    };
+    struct Implementation : public Interface {
+      void Func() override {
+          $Callee[[callee]]();
+      }
+    };
+    void Test(Interface& cls){
+      cls.$FuncCall[[Func]]();
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+  auto Index = TU.index();
+
+  std::vector<CallHierarchyItem> Items =
+      prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
+  ASSERT_THAT(Items, ElementsAre(withName("callee")));
+  auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
+  ASSERT_THAT(
+      IncomingLevel1,
+      ElementsAre(AllOf(from(AllOf(withName("Func"), withDetail("Implementation::Func"))),
+                        iFromRanges(Source.range("Callee")))));
+  auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
+  ASSERT_THAT(
+      IncomingLevel2,
+      ElementsAre(AllOf(from(AllOf(withName("Test"), withDetail("Test"))),
+                        iFromRanges(Source.range("FuncCall")))));
+
+  auto IncomingLevel3 = incomingCalls(IncomingLevel2[...
[truncated]

Copy link

github-actions bot commented Oct 12, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this! I haven't had a super detailed look yet but I had a quick read-through and this seems to be on the right track.

A couple of high-level pieces of feedback:

  • It would be nice to quantify the impact of this change on the index's memory footprint. VSCode has a handy clangd: Show memory usage command for that. Could you check the size of the background_index memory size reported there before/after this patch, on a codebase large enough to be representative (llvm-project works, or feel free to use something else of comparable size that you're working on)?
  • We have another implementation of SymbolIndex here, used for the remote index, which is only built when clangd is built with -DCLANGD_ENABLE_REMOTE=ON in the CMake flags. It looks like the pre-commit CI does not set this flag, but some of the buildbots (post-commit CI) do, so we'll need to get the project to build in this configuration as well.
    • In terms of actual implementation, if you have the interest / familiarity with protocol buffers to plumb in an actual implementation then feel free to do so, otherwise I would say it's also fine to leave it as a stub to be implemented in a future enhancement. (But it still needs to compile.)

Also cc @kadircet as a heads up about the new SymbolIndex API here.

@timon-ul
Copy link
Contributor Author

It would be nice to quantify the impact of this change on the index's memory footprint.

Ok so first off, I tried to compare the memory usage with our project at work, which is quite a bit larger than llvm and the numbers kinda surprise me, because they are basically the same (yes I made sure that in one of them the callHierarchy does not work and in the other one it does). Left is before my changes, right is after:
grafik

Not sure if I am doing something wrong....

We have another implementation of SymbolIndex here

Uh, interesting, I used clangd's FindImplementation feature to find all of them but this one did not show up, maybe it is because my clangd cannot find the grpc header and the whole file is just red (and I guess it cannot find that one since I have not compiled with -DCLANGD_ENABLE_REMOTE=ON, gonna check that).

if you have the interest / familiarity with protocol buffers

Not really familiar beyond surface level, but sure why not.

@timon-ul
Copy link
Contributor Author

Just as a sanity check I also did it with the llvm repository, same result, no visible difference.

grafik

@HighCommander4
Copy link
Collaborator

Ok so first off, I tried to compare the memory usage with our project at work, which is quite a bit larger than llvm and the numbers kinda surprise me, because they are basically the same

Ah, we have to extend Dex::estimateMemoryUsage() to actually count ReverseRelations.

We have another implementation of SymbolIndex here

Uh, interesting, I used clangd's FindImplementation feature to find all of them but this one did not show up, maybe it is because my clangd cannot find the grpc header and the whole file is just red (and I guess it cannot find that one since I have not compiled with -DCLANGD_ENABLE_REMOTE=ON, gonna check that).

Right, build without -DCLANGD_ENABLE_REMOTE=ON --> Client.cpp does not get built --> Client.cpp does not appear in compile_commands.json --> clangd does not index it.

@timon-ul
Copy link
Contributor Author

timon-ul commented Oct 14, 2025

Ok let's start with memory usage, seems like the increase really is miniscule, less than 1.5%, does not look like a problematic amount to me.

grafik

@timon-ul
Copy link
Contributor Author

Ok so I am really not sure what else needs to be implemented, compiling with the -DCLANGD_ENABLE_REMOTE=ON option works no problem, but I tried to run check-clangd and it did not even finish the compile (or linking, not sure). This behaviour is the same without my changes though so not really my fault I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants