[PATCH] D62839: [clangd] Index API and implementations for relations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:293
+  // This map is used to figure out where to store relations.
+  llvm::DenseMap<SymbolID, File *> IDsToFiles;
   for (const auto &Sym : *Index.Symbols) {
----------------
nit: rename to `SymbolIDToFile`?


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:329
+    const auto FileIt = IDsToFiles.find(Rel.Subject);
+    if (FileIt != IDsToFiles.end()) {
+      FileIt->second->Relations.insert(&Rel);
----------------
nit: braces


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:201
+  std::vector<Relation> AllRelations;
+  {
+    for (const auto &RelationSlab : RelationSlabs) {
----------------
redundant scope


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:225
         std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
-                        std::move(RefsStorage), std::move(SymsStorage)),
+                        std::move(RelationSlabs), std::move(RefsStorage),
+                        std::move(SymsStorage)),
----------------
no need to pass `RelationSlabs` in here `AllRelations` are just value types.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:250
       Path, llvm::make_unique<SymbolSlab>(std::move(Symbols)),
-      llvm::make_unique<RefSlab>(), /*CountReferences=*/false);
+      llvm::make_unique<RefSlab>(), llvm::make_unique<RelationSlab>(),
+      /*CountReferences=*/false);
----------------
I think we need to pass relationslab in here. Since we might miss relations like the following that are outside the main file:
```
class A {};
class B : public A {};
```
Would be glad if you could prove me right/wrong with a unittest as well.


================
Comment at: clang-tools-extra/clangd/index/Index.h:107
+  virtual void
+  relations(const SymbolID &Subject, index::SymbolRole Predicate,
+            llvm::function_ref<void(const SymbolID &)> Callback) const = 0;
----------------
We might need additional options, like limiting number of results. Could you instead accept a `RelsRequest` object? You can check `RefsRequest` for a sample


================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:22
   // Store Slab size before it is moved.
   const auto BackingDataSize = Slab.bytes() + Refs.bytes();
+  auto Data =
----------------
`+ Relations.bytes()`


================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:24
+  auto Data =
+      std::make_tuple(std::move(Slab), std::move(Refs), std::move(Relations));
+  return llvm::make_unique<MemIndex>(std::get<0>(Data), std::get<1>(Data),
----------------
no need to put `Relations` into `Data` they are just values, right?


================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:94
+  auto Rels = Relations.lookup(Subject, Predicate);
+  for (const auto &R : Rels) {
+    Callback(R.Object);
----------------
nit: braces


================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:29
       this->Refs.try_emplace(R.first, R.second.begin(), R.second.end());
+    RelationSlab::Builder Builder;
+    for (const Relation &R : Relations)
----------------
why not just store `densemap<<s,p>,arrayref<o>>` ?


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:126
+    llvm::function_ref<void(const SymbolID &)> Callback) const {
+  // Return results from both indexes but avoid duplicates.
+  llvm::DenseSet<SymbolID> SeenObjects;
----------------
avoiding deduplicates is not enough, we also wouldn't want to report stale relations coming from static index.

Could you check the logic in `MergedIndex::refs`, and hopefully refactor it into a class to share between these two?


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:28
+                                        RelationSlab Rels) {
   auto Size = Symbols.bytes() + Refs.bytes();
+  // There is no need to include "Rels" in Data because the relations are self-
----------------
`+rels.size()`


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:269
+  auto Rels = Relations.lookup(Subject, Predicate);
+  for (const auto &R : Rels) {
+    Callback(R.Object);
----------------
nit: braces


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.h:110
   llvm::DenseMap<SymbolID, llvm::ArrayRef<Ref>> Refs;
+  RelationSlab Relations;
   std::shared_ptr<void> KeepAlive; // poor man's move-only std::any
----------------
let's store `densemap<<s,p>, arrayref<o>>` here as well


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62839/new/

https://reviews.llvm.org/D62839