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