sammccall added a comment.
Thanks for sending this early!
Rough interface comments - mostly looks good though!
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:26
+
+using PostingList = std::vector<size_t>;
+
----------------
we should at least use a type alias for a DocID (maybe an opaque one - not sure
for now)
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:39
+public:
+ virtual bool reachedEnd() const = 0;
+ // FIXME(kbobyrev): Should advance() and advanceTo() return size_t peek()
----------------
or have peek return an InvalidID == size_t{-1}?
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:43
+ virtual bool advance() = 0;
+ virtual bool advanceTo(size_t Rank) = 0;
+ virtual size_t peek() const = 0;
----------------
name Rank is confusing. Type should be DocID (param doesn't need a name in the
interface)
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:44
+ virtual bool advanceTo(size_t Rank) = 0;
+ virtual size_t peek() const = 0;
+
----------------
could reasonably call this operator*
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:53
+public:
+ DocumentIterator(std::shared_ptr<PostingList> Documents);
+
----------------
is shared_ptr really needed here? Seems like a raw pointer would be fine. I can
see that when rebuilding the index we'll have some fun lifetime issues that are
sometimes solved with shared_ptr, but I think that would be at the full-index
level, not the individual-posting-list level.
(if you want one less indirection, you could have `using PostingListRef =
ArrayRef<size_t>`, but it shouldn't matter)
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:59
+ bool advanceTo(size_t Rank) override;
+ size_t getIndex() const;
+ size_t peek() const override;
----------------
what's this for?
I don't think it's worth exposing for testing, should be able to get at the
needed cases through the public interface
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:63
+ ///
+ void reset();
+
----------------
how will you use this? it's not in the interface, and if you know the
implementation then creating a new iterator is cheap
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:76
+public:
+ AndIterator(const std::vector<std::shared_ptr<QueryIterator>> &Children);
+
----------------
again, unless you have a strong reason, don't use shared_ptr, as it makes it
hard to reason about ownership and lifetimes.
These are a tree, unique_ptr should be fine. That way a top-level owned
iterator is self-contained.
================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:83
+
+ const std::vector<std::shared_ptr<QueryIterator>> getChildren() const;
+
----------------
what's this for?
https://reviews.llvm.org/D49546
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
