sammccall added a comment.
A few more comments about the bits I understand, but waiting mostly on the
documentation.
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:155
+ llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+ OS << "(&& ";
+ for (size_t Index = 0; Index < Children.size(); ++Index) {
----------------
nit: `&` should be enough?
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:175
+// FIXME(kbobyrev): This compares IDs of two iterators...
+auto CompareIterators = [](std::unique_ptr<QueryIterator> &Left,
+ std::unique_ptr<QueryIterator> &Right) -> bool {
----------------
can this just be a static function?
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:196
+ });
+ std::make_heap(begin(Children), end(Children), CompareIterators);
+ }
----------------
can we start with the simple/obvious implementation, and add heap later if it
actually improves performance?
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:257
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+ const QueryIterator &Iterator) {
----------------
you can just inline this.
If you don't, you have to provide a declaration in the header, outside the
class definition. (In addition to the friend declaration). GCC errors on this,
clang ignores it (GCC is right).
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:28
+
+using DocID = size_t;
+using PostingList = std::vector<DocID>;
----------------
`uint32_t` should be plenty. These will be a large fraction of the index's
memory usage. (Especially if we push actual payloads onto disk later)
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:41
+// FIXME(kbobyrev): Document this one properly.
+// FIXME(kbobyrev): Don't expose all the classes in the header, move classes to
+// header and only have constructors here.
----------------
this is done I think.
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:43
+// header and only have constructors here.
+class QueryIterator {
+public:
----------------
I'm not sure `Query` actually adds much to this class name. `Iterator`,
`AndIterator`, etc would read more smoothly.
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:47
+ // instead?
+ virtual bool reachedEnd() const = 0;
+ virtual bool advance() = 0;
----------------
nit: this seems non-orthogonal, could we either:
- have `advance()` and `advanceTo()` return void
- or remove `reachedEnd()` and have owners that need to keep the state (and/or
iterators) maintain it separately?
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:52
+ virtual DocID peek() const = 0;
+ virtual size_t cost() const = 0;
+
----------------
As discussed offline, please leave this out of the initial patch.
It requires quite some explanation and understanding, and seems likely to delay
the review.
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:54
+
+ virtual llvm::raw_ostream &dump(llvm::raw_ostream &OS) const = 0;
+
----------------
private?
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:68
+std::unique_ptr<QueryIterator>
+constructAndIterator(std::vector<std::unique_ptr<QueryIterator>> &&Children);
+
----------------
nit: `newAndIterator` or just `andIterator`? brevity is the soul of wit and all
that
Or even better, what about:
```
Iterator::createAnd(...)
Iterator::createOr(...)
Iterator::create(PostingList*)
```
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:68
+std::unique_ptr<QueryIterator>
+constructAndIterator(std::vector<std::unique_ptr<QueryIterator>> &&Children);
+
----------------
sammccall wrote:
> nit: `newAndIterator` or just `andIterator`? brevity is the soul of wit and
> all that
>
> Or even better, what about:
>
> ```
> Iterator::createAnd(...)
> Iterator::createOr(...)
> Iterator::create(PostingList*)
> ```
nit: pass by value rather than by rvalue-reference unless there's a strong
reason.
Here, vector<u_p> isn't copyable, and there are no overloads, and this isn't
performance-critical, so it should be fine.
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:19
+
+using std::make_shared;
+using std::max;
----------------
please spell these out (local style)
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:27
+
+using llvm::raw_string_ostream;
+
----------------
using namespace llvm
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:67
+
+ vector<unique_ptr<QueryIterator>> Children;
+
----------------
nit: just `auto And =
constructAndIterator({constructDocumentIterator(EmptyList)})`
(and below in various places).
This actually seems nicer - the expression hierarchy reflects the iterator
hierarchy.
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:75
+
+ string Dump;
+ raw_string_ostream OS(Dump);
----------------
just `EXPECT_EQ(to_string(*And), "(&& [])")`
You may need to `#include "llvm/Support/ScopedPrinters.h"`
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:98
+
+ EXPECT_EQ(And->reachedEnd(), false);
+ EXPECT_EQ(And->peek(), 0U);
----------------
add a helper function `vector<DocID> consume(Iterator&)`
(Maybe even to the main file rather than the tests).
Then this section (and some others) can just be `EXPECT_THAT(consume(*And),
ElementsAre(0u,7u,10u,...))`
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:140
+
+ // More complicated example: nested And iterators.
+ //
----------------
Hmm, I'm not sure this approach scales - there shouldn't be anything special
about composing and/and, compared to and/or, or/not, boost/and, etc.
I'd suggest testing each iterator directly, and then having one test that uses
a more complex tree, and dropping this case.
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
+TEST(DexIndexIterators, QueryTree) {
+ // An example of more complicated query
+ //
----------------
This is too complicated. Try to use each iterator once or twice.
(We'll need to extend this when we add new iterator types)
https://reviews.llvm.org/D49546
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
