[PATCH] D49546: [clangd] Proof-of-concept query iterators for Dex symbol index

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lg! just a few nits.



================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109
+private:
+  /// Restores class invariants: each child should point to the same element.
+  void sync() {
----------------
This is the post-condition, not a precondition right?

To be clearer, ` ... each child will point to the same element after sync`




================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:162
+
+  /// Returns false if any child is not exhausted, true otherwise.
+  bool reachedEnd() const override {
----------------
Or `Returns true if all children are exhausted.` No need for otherwise if it's trivial.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:12
+// symbol, such as high fuzzy matching score, scope, type etc. The lists of all
+// symbols matching some criteria (e.g. belonging to "clang::clangd" scope) are
+// expressed in a form of Search Tokens which are stored in the inverted index.
----------------
nit: Just to match the actual implementation, use "clang::clangd::" to avoid confusion.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:15
+// Inverted index maps these tokens to the posting lists - sorted (e.g. by
+// number of references) sequences of symbol IDs matching the token, e.g. scope
+// token "clangd::clangd" is mapped to the list of IDs of all symbols which are
----------------
"number of references" is too specific. Maybe "by symbol quality"?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:25
+// because it allows receiving a certain number of the most valuable items (e.g.
+// symbols with most number of references if that is the sorting key in the
+// first place) without processing all items with requested qualities (this
----------------
Again, replace number of references with quality for generalization.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:26
+// symbols with most number of references if that is the sorting key in the
+// first place) without processing all items with requested qualities (this
+// might not be computationally effective if search request is not very
----------------
with the requested *properties*?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:61
+/// to be extensible in order to support multiple types of iterators, some of
+/// which are not yet introduced.
+class Iterator {
----------------
nit: drop "some of which are not yet introduced" as it's not relevant to the interface itself.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:72
+  /// crash, false otherwise.
+  virtual bool reachedEnd() const = 0;
+  /// Moves to next valid DocID. If it doesn't exist, the iterator is exhausted
----------------
Just "return true if ...". 

There is no need to mention `advance()` and `advanceTo()` here as they are already covered in their own documentation. 


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:76
+  ///
+  /// Note: calls to advance() would trigger assertion (if enabled) or crash if
+  /// reachedEnd().
----------------
Just `reachedEnd() must be false.` which usually implies assertion. Same elsewhere.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:127
+
+/// Allows calling createAnd() with {} braces by constructing an array of
+/// r-values rather than an initializer list.
----------------
An example of usage would be simpler and easier to understand here. Same below.

Example:
`This allows createAnd({create(...), create(...)})`


https://reviews.llvm.org/D49546