[PATCH] D62459: [clangd] Serialization support for RelationSlab

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:29
+
+llvm::Expected<RelationKind> symbolRoleToRelationKind(index::SymbolRole Role) {
+  // SymbolRole is used to record relations in the index.
----------------
no need for errors, use `llvm_unreachable` instead.


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:45
+
+llvm::Expected<index::SymbolRole> relationKindToSymbolRole(RelationKind Kind) {
+  switch (Kind) {
----------------
same for error


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:79
+  // A caller can set the error bit if an invalid value was read.
+  void setErr() { Err = true; }
   // Did we read all the data, or encounter an error?
----------------
I believe reader should not care about the high level representation of data. therefore error state should be internal and only set if the low level read has failed.

high level reads are out of `Reader`s scope and should be propagated by the caller.


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:394
 
+// RELATIONS ENCODING
+// A relations section is a flat list of relations. Each relation has:
----------------
could you also add a comment saying, we might prefer a packed representation if need arises in future?


================
Comment at: clang-tools-extra/clangd/index/Serialization.h:81
+// Used for serializing SymbolRole as used in Relation.
+enum class RelationKind : uint8_t { ChildOf = 1, BaseOf };
+llvm::Expected<RelationKind> symbolRoleToRelationKind(index::SymbolRole);
----------------
why not start from zero?

Also let's change the `index::SymbolRole` in `Relation` with this one.


================
Comment at: clang-tools-extra/clangd/index/Serialization.h:81
+// Used for serializing SymbolRole as used in Relation.
+enum class RelationKind : uint8_t { ChildOf = 1, BaseOf };
+llvm::Expected<RelationKind> symbolRoleToRelationKind(index::SymbolRole);
----------------
kadircet wrote:
> why not start from zero?
> 
> Also let's change the `index::SymbolRole` in `Relation` with this one.
why not just store baseof ? do we need both directions for typehierarchy?


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:225
 
+const char *RelationsYAML = R"(
+--- !Symbol
----------------
let's incorporate these data into existing yaml string at top of the file, I believe you just need to add ```--- !Relations``` section


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62459