[PATCH] D59407: [clangd] Add RelationSlab

This revision was automatically updated to reflect the committed changes. Closed by commit rL362352: [clangd] Add RelationSlab (authored by nridge, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D59407: [clangd] Add RelationSlab

kadircet accepted this revision. kadircet added a comment. Still LG, thanks for the patch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407 ___ cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

nridge updated this revision to Diff 202369. nridge added a comment. Address latest review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407 Files: clang-tools-extra/clangd/CMakeLists.txt

[PATCH] D59407: [clangd] Add RelationSlab

kadircet added a comment. btw, I believe you have enough good quality patches to apply for commit access. Are there any obstacles that is keeping you from applying? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/

[PATCH] D59407: [clangd] Add RelationSlab

sammccall accepted this revision. sammccall added a comment. Awesome, thank you! Comment at: clang-tools-extra/clangd/index/Relation.h:75 + private: +std::vector Relations; + }; kadircet wrote: > maybe use a set so that we can be sure that there won't be

[PATCH] D59407: [clangd] Add RelationSlab

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, except the duplication issue. Thanks! Comment at: clang-tools-extra/clangd/index/Relation.h:75 + private: +std::vector Relations; + }; maybe

[PATCH] D59407: [clangd] Add RelationSlab

nridge added inline comments. Comment at: clang-tools-extra/clangd/index/Relation.cpp:19 +llvm::iterator_range +RelationSlab::lookup(const SymbolID , + index::SymbolRole Predicate) const { kadircet wrote: > I would suggest adding another

[PATCH] D59407: [clangd] Add RelationSlab

nridge updated this revision to Diff 201427. nridge marked 19 inline comments as done. nridge added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407 Files:

[PATCH] D59407: [clangd] Add RelationSlab

kadircet added a comment. Mostly LG from my side, thanks! Comment at: clang-tools-extra/clangd/index/Relation.cpp:19 +llvm::iterator_range +RelationSlab::lookup(const SymbolID , + index::SymbolRole Predicate) const { I would suggest adding

[PATCH] D59407: [clangd] Add RelationSlab

nridge updated this revision to Diff 200735. nridge added a comment. Herald added a subscriber: mgrang. Implemented discussed design approach ("Add a RelationSlab storing (subject, predicate, object) triples, intended for sparse relations") Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D59407: [clangd] Add RelationSlab

nridge planned changes to this revision. nridge added a comment. In D59407#1478794 , @sammccall wrote: > So if you can stomach it, I think > > > **Approach 2: Add a RelationSlab storing (subject, predicate, object) > > triples, intended for sparse

[PATCH] D59407: [clangd] Add RelationSlab

sammccall added a subscriber: hokein. sammccall added a comment. Hi Nathan, sorry for the stall here, and for repeatedly going over the same issues. The design space here is pretty complicated. I think the conclusion of recent offline discussions is: - refs and relations can be the same

[PATCH] D59407: [clangd] Add RelationSlab

nridge added a comment. Hi, any update here? I would appreciate some guidance so I can move forward with this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407

[PATCH] D59407: [clangd] Add RelationSlab

nridge added a comment. In D59407#1456394 , @sammccall wrote: > One thing that strikes me here is that this case is very similar to our > existing Ref data - it's basically a subset, but with a symbolid payload > instead of location. We could consider

[PATCH] D59407: [clangd] Add RelationSlab

sammccall added a comment. In D59407#1447070 , @nridge wrote: > @sammccall, thank you for having a look at this. > > I have no objection to revising the data model if there's agreement on a > better one. > > In D59407#1446464

[PATCH] D59407: [clangd] Add RelationSlab

nridge requested review of this revision. nridge added a reviewer: sammccall. nridge added a comment. I guess I should clear the "Accepted" status until we settle the question of the data model. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D59407: [clangd] Add RelationSlab

nridge added a comment. @sammccall, thank you for having a look at this. I have no objection to revising the data model if there's agreement on a better one. In D59407#1446464 , @sammccall wrote: > - I don't think we yet know what the more

[PATCH] D59407: [clangd] Add RelationSlab

sammccall added a comment. (Sorry to arrive late at this discussion, I'm just back from leave. I have some suggestions and would appreciate your thoughts, but if simply this feels too much like going around in circles I'm happy to work out how we can address this after this patch lands instead.

[PATCH] D59407: [clangd] Add RelationSlab

nridge added a comment. Ok, cool. In that case, I think this patch is ready to be committed, and would appreciate it if someone could commit it. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407

[PATCH] D59407: [clangd] Add RelationSlab

gribozavr added a comment. Submitting code as it becomes ready is the usual practice here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407 ___ cfe-commits

[PATCH] D59407: [clangd] Add RelationSlab

nridge added a comment. As this is the first of a series of patches adding support for relations, and then building type hierarchy subtypes on top (as discussed here ), how should we go about landing this -- should we land each patch in the series as

[PATCH] D59407: [clangd] Add RelationSlab

nridge updated this revision to Diff 191886. nridge added a comment. Scrapped 'struct Relation' and addressed other comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59407/new/ https://reviews.llvm.org/D59407 Files:

[PATCH] D59407: [clangd] Add RelationSlab

nridge marked 2 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/index/Relation.h:1 +//===--- Ref.h ---*- C++-*-===// +// gribozavr wrote: > gribozavr wrote: > > "---

[PATCH] D59407: [clangd] Add RelationSlab

gribozavr added inline comments. Comment at: clang-tools-extra/clangd/index/Relation.h:1 +//===--- Ref.h ---*- C++-*-===// +// gribozavr wrote: > "--- Relation.h " Not done?

[PATCH] D59407: [clangd] Add RelationSlab

nridge marked 8 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/index/Index.h:43 +public: + using value_type = std::pair>; + using const_iterator = std::vector::const_iterator; kadircet wrote: > nridge wrote: > >

[PATCH] D59407: [clangd] Add RelationSlab

kadircet added a comment. In D59407#1432543 , @nridge wrote: > If it's (B), how many bytes should the index be? Are the space gains worth > the complexity, given that `SymbolID` is only 8 bytes to begin with? (As > compared to say, the filenames in

[PATCH] D59407: [clangd] Add RelationSlab

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/Index.h:59 +return sizeof(*this) + Arena.getTotalMemory() + + sizeof(value_type) * Relations.size(); + }

[PATCH] D59407: [clangd] Add RelationSlab

nridge updated this revision to Diff 191036. nridge marked an inline comment as done. nridge added a comment. Herald added a subscriber: mgorny. Address review comments, except for the deduplication which is still under discussion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D59407: [clangd] Add RelationSlab

nridge marked 7 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/index/Index.h:43 +public: + using value_type = std::pair>; + using const_iterator = std::vector::const_iterator; kadircet wrote: > gribozavr wrote: > >

[PATCH] D59407: [clangd] Add RelationSlab

nridge added a comment. In D59407#1430656 , @kadircet wrote: > 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

[PATCH] D59407: [clangd] Add RelationSlab

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/Index.h:43 +public: + using value_type = std::pair>; + using const_iterator = std::vector::const_iterator; gribozavr wrote: > `struct Relation`? And in the comments for it, please

[PATCH] D59407: [clangd] Add RelationSlab

gribozavr added inline comments. Comment at: clang-tools-extra/clangd/index/Index.cpp:35 +auto *Array = Arena.Allocate(Rels.size()); +std::uninitialized_copy(Rels.begin(), Rels.end(), Array); +Result.emplace_back(Entry.first, Use `ArrayRef::copy()`,

[PATCH] D59407: [clangd] Add RelationSlab

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

[PATCH] D59407: [clangd] Add RelationSlab

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/index/Index.h:25 +enum class RelationKind { Subtype }; + One thing I'm wondering about is: would it be better to just use `clang::index::SymbolRole`

[PATCH] D59407: [clangd] Add RelationSlab

nridge created this revision. nridge added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. Herald added a project: clang. RelationSlab is a new index data structure, similar to SymbolSlab and RefSlab. RelationSlab stores relations