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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:202
+  for (const auto &RelationSlab : RelationSlabs) {
+    for (const auto &R : *RelationSlab) {
+      AllRelations.push_back(R);
----------------
nit: braces


================
Comment at: clang-tools-extra/clangd/index/Index.h:76
 
+struct RelationsRequest {
+  SymbolID Subject;
----------------
let's have a limit as well


================
Comment at: clang-tools-extra/clangd/index/Index.h:139
+  void relations(const RelationsRequest &,
+                 llvm::function_ref<void(const SymbolID &)>) const override;
+
----------------
thinking about this callback, I don't think there is any user that will make use of the `SymbolID`s without querying for a `Symbol`.

So to reduce this boilerplate let's rather move this lookup into this API(all `SymbolIndex` implementations provide `lookup` anyway so they all have a way to convert a `SymbolID` into a `Symbol`) and change the callback to accept a `const Symbol&` instead of a `SymbolID`


================
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;
----------------
nridge wrote:
> kadircet wrote:
> > We might need additional options, like limiting number of results. Could you instead accept a `RelsRequest` object? You can check `RefsRequest` for a sample
> I called it `RelationsRequest` to avoid visual confusion with `RefsRequest`.
yeah that's a good call thanks!


================
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;
----------------
nridge wrote:
> kadircet wrote:
> > 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?
> The description in `MergedIndex::refs()` says:
> 
> ```
>   // We don't want duplicated refs from the static/dynamic indexes,
>   // and we can't reliably deduplicate them because offsets may differ slightly.
>   // We consider the dynamic index authoritative and report all its refs,
>   // and only report static index refs from other files.
> ```
> 
> It seems to me that the problem of "can't reliably deduplicate them because offsets may differ slightly" doesn't apply to relations, as there are no offsets involved. So, is there still a need to have additional logic here?
> 
> If so, what would the logic be -- don't return an object O1 from the static index if we've returned an object O2 from the dynamic index defined in the same file as O1? (Note that to implement this, we'd have to additionally look up each SymbolID we return in the symbol table, as we don't otherwise know what file the object is located in.)
yes I was referring to second part actually, but you are right we have no good way of distinguishing stale information in this case. Let's leave it as it is, but add a comment stating that we might return stale relations from static index.


================
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-
----------------
nridge wrote:
> kadircet wrote:
> > `+rels.size()`
> `Size` is only the backing data size, so if we don't include the relations in the backing data, we shouldn't include `Rels.size()`, right?
ok, but then let's make sure we include size of the densemap in `estimateMemoryUsage` manually.


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:220
+  // containing the definition of the subject (A_CC).
+  EXPECT_EQ(ShardHeader->Relations->size(), 1u);
 
----------------
could we instead check only relation is `A_CC isbaseof B_CC` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62839