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