[PATCH] D51982: [clangd] Introduce PostingList interface

[Sam McCall via Phabricator via cfe-commits]

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