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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
