[PATCH] D59407: [clangd] Add RelationSlab

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

This mostly looks good, one high level comment:
I believe it makes sense to deduplicate SymbolIDs for RelationSlab.
Up until now, we mostly had only one occurence of a SymbolID in a Slab, but RelationSlab does not follow that assumption.

Also can you add a few tests after the above mentioned check to make sure interning SymbolIDs works as expected?



================
Comment at: clang-tools-extra/clangd/index/Index.h:25
 
+enum class RelationKind { Subtype };
+
----------------
nridge wrote:
> One thing I'm wondering about is: would it be better to just use `clang::index::SymbolRole` here (which has `Relation___Of` entries that cover what we're interested in) instead of having our own enum?
I totally agree, but that struct is 4 bytes. I am not sure it is worth the trade off when storing the relationslab, but since this patch is not related to serialization let's get rid of RelationKind and make use of clang::index::SymbolRole.

When landing serialization part we can either split clang::index::SymbolRole into two parts or add some custom mapping to serialize only relevant bits etc.


================
Comment at: clang-tools-extra/clangd/index/Index.h:59
+    return sizeof(*this) + Arena.getTotalMemory() +
+           sizeof(value_type) * Relations.size();
+  }
----------------
use capacity instead of size


================
Comment at: clang-tools-extra/clangd/index/Index.h:67
+    Builder() {}
+    // Adds a relation to the slab. Deep copy: Strings will be owned by the
+    // slab.
----------------
I am not sure comment currently applies to RelationSlab internals, since everything is of value type anyways, there are no references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407