sammccall added a comment.
This looks really nice.
Iterator implementations can be simplified a bit I think.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:50
+ OS << Documents[Index];
+ if (Index + 1 != Documents.size())
+ OS << ", ";
----------------
(uber-nit: slightly clearer to add the separator first, then you only have to
compare against zero.
Or this idiom:
```const char* Sep = "";
for (DocID D : Documents) {
OS << Sep << D;
Sep = ", ";
}
```
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:59
+ PostingListRef Documents;
+ DocID Index;
+};
----------------
nit: the code might be slightly more concise (particularly advanceTo) if the
current element is represented by a PostingListRef::Iterator rather than an
index, up to you.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:59
+ PostingListRef Documents;
+ DocID Index;
+};
----------------
sammccall wrote:
> nit: the code might be slightly more concise (particularly advanceTo) if the
> current element is represented by a PostingListRef::Iterator rather than an
> index, up to you.
(DocID is the wrong type for an index)
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:65
+public:
+ AndIterator(std::vector<std::unique_ptr<Iterator>> &&AllChildren)
+ : Children(std::move(AllChildren)), ReachedEnd(false) {
----------------
nit: just take children by value without &&
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:69
+ DocID ID = 0;
+ for (auto &Child : Children) {
+ // Check whether any child iterator points to the end. In this case,
----------------
nit: const auto &
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:74
+ ReachedEnd = true;
+ break;
+ }
----------------
just return so you don't need the if at the end?
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:88
+ // FIXME(kbobyrev): Properly document this one.
+ void advance() override {
+ assert(!reachedEnd() && "AndIterator can't call advance() at the end.");
----------------
It would be nice if the functions in this class could share some logic.
what about:
```
for (auto& C : children)
C->advance();
sync();
```
where `sync` is
```
// restores class invariants: ReachedEnd set, or all children in sync
void sync() {
DocID Max = 0;
for (auto &C : Children) {
if (C->reachedEnd()) {
ReachedEnd = true;
return;
}
Max = std::max(C->peek(), Max);
}
for (auto &C : Children) {
C->advanceTo(Max);
if (C->reachedEnd()) {
ReachedEnd = true;
return;
}
}
}
```
the trick is then the constructor is just `sync();`, `advanceTo` is just
"advance all children and sync", etc.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:155
+// FIXME(kbobyrev): Document this one properly.
+// FIXME(kbobyrev): Currently, the implementation is inefficient because peek()
+// and reachedEnd() are computed in O(Children.size()). This can be fixed by
----------------
I think this is `FIXME: would a min-heap be faster?` :-)
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:181
+ const auto HighestID = peek();
+ DocID ChildrenToAdvance = 0;
+ while ((ChildrenToAdvance++ < Children.size()) && !reachedEnd() &&
----------------
I can't follow this one - why is the outer loop needed?
(It looks like you're using DocID as integer again, but I can't tell why the
variable is needed at all)
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:184
+ (peek() == HighestID)) {
+ for (auto &Child : Children) {
+ if (!Child->reachedEnd() && Child->peek() == HighestID) {
----------------
nit: you can drop the extra braces here and in advanceTo (at least, you use
that style above)
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:204
+ "OrIterator must have at least one child to peek().");
+ bool FoundFirstValid = false;
+ DocID Result = 0;
----------------
why not just initialize Result to `std::numeric_limits<DocID>::max()`, and skip
the valid checking?
You know by assertion that you'll find something better.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:235
+ std::vector<DocID> Result;
+ while (!It.reachedEnd()) {
+ Result.push_back(It.peek());
----------------
`for (; !It.reachedEnd(); It.advance())`
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:50
+
+ static std::vector<DocID> consume(Iterator &It);
+
----------------
this doesn't really make sense as static as it operates on an Iterator.
Either a free function or non-static member seems fine
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:65
+ static std::unique_ptr<Iterator>
+ createAnd(std::vector<std::unique_ptr<Iterator>> Children);
+
----------------
nit nit: move above the template overload so you can document the readable one.
https://reviews.llvm.org/D49546
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
