[PATCH] D59407: [clangd] Add RelationSlab

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Mostly LG from my side, thanks!



================
Comment at: clang-tools-extra/clangd/index/Relation.cpp:19
+llvm::iterator_range<RelationSlab::iterator>
+RelationSlab::lookup(const SymbolID &Subject,
+                     index::SymbolRole Predicate) const {
----------------
I would suggest adding another version which returns all the relations a given `Subject` participates, but I suppose currently we don't have any use cases.

Feel free to add or skip, that can be added when necessary.


================
Comment at: clang-tools-extra/clangd/index/Relation.cpp:24
+                                   [](const Relation &A, const Relation &B) {
+                                     return (A.Subject < B.Subject) ||
+                                            (A.Subject == B.Subject &&
----------------
`std::tie`


================
Comment at: clang-tools-extra/clangd/index/Relation.h:31
+  bool operator==(const Relation &Other) const {
+    return Subject == Other.Subject && Predicate == Other.Predicate &&
+           Object == Other.Object;
----------------
nit: could you use `std::tie`


================
Comment at: clang-tools-extra/clangd/index/Relation.h:35
+  // SPO order
+  bool operator<(const Relation &Other) const {
+    if (Subject < Other.Subject) {
----------------
again `std::tie`


================
Comment at: clang-tools-extra/clangd/index/Relation.h:52
+
+class RelationSlab {
+public:
----------------
I believe `RelationSlab` should still be immutable, even after the `build` operation below `RelationSlab` is mutable.
Let's introduce the builder class and move mutating operations and build into it. So that it would be more symmetric to other slabs we have.

Also are there any use-cases requiring RelationSlab to be mutable?


================
Comment at: clang-tools-extra/clangd/index/Relation.h:60
+  // that possibility for the future. Having a build() method is
+  // also useful so when know when to sort the relations.
+  using Builder = RelationSlab;
----------------
there seems to be some typos in the comment


================
Comment at: clang-tools-extra/clangd/index/Relation.h:72
+
+  void push_back(const Relation &R) { Relations.push_back(R); }
+
----------------
maybe rename to `insert` to keep symmetry with other slabs?
also it suggests a semantics on the operation that we don't care about.


================
Comment at: clang-tools-extra/clangd/index/Relation.h:79
+  /// Sort relations in SPO order.
+  void sort();
+
----------------
maybe even get rid of sort by inlining it into build?


================
Comment at: clang-tools-extra/clangd/index/Relation.h:82
+  /// Lookup all relations matching the given subject and predicate.
+  /// Assumes the slab is sorted in SPO order.
+  llvm::iterator_range<iterator> lookup(const SymbolID &Subject,
----------------
again, we could get rid of this comment if slab was immutable


================
Comment at: clang-tools-extra/clangd/index/Relation.h:89
+private:
+  RelationSlab(std::vector<Relation> Relations)
+      : Relations(std::move(Relations)) {}
----------------
Do we really need this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407