sammccall added a comment. At a high level: making posting lists an abstract type adds another layer of indirection, which we can use to solve problems. It also has costs, mostly conceptual complexity. What problems are we solving?
- if **we want to dynamically use a different representation for different data/scenarios**: this would be a good solution, do we have any evidence that this is the case? I thought the tentative conclusion was vbyte was good enough for all cases. What's the deal with dense/sparse? - if **it's too hard to experiment with different posting list types**: how hard is it really? I'm not sure this justifies checking this change in. I'd suggest as a first step making PostingList a concrete class - this would improve the API and enable experimentation. (Even experimentation with dynamically switching the implementation - it's easier behind a class boundary) ================ Comment at: clang-tools-extra/clangd/index/dex/PostingList.h:34 + +/// PostingList is an interface for the storage of DocIDs which can be inserted +/// to the Query Tree as a leaf by constructing Iterator over given PostingList. ---------------- this talks a lot about how a posting list can be implemented, and what it interacts with - I'd suggest removing/reducing that and talking about what it is, instead :-) ================ Comment at: clang-tools-extra/clangd/index/dex/PostingList.h:54 + +/// Returns posting list with sparse in-memory representation. This is useful +/// for small size posting lists with huge gaps between subsequent DocIDs. ---------------- why is the caller responsible for choosing the implementation? It seems like buildPostingList could do this based on inspecting the docs ================ Comment at: clang-tools-extra/clangd/index/dex/PostingList.h:61 +std::unique_ptr<PostingList> +buildPlainPostingList(const std::vector<DocID> &&Documents); + ---------------- PostingList::create()? https://reviews.llvm.org/D51982 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
