sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109
+ float consume() override {
+ if (reachedEnd())
return DEFAULT_BOOST_SCORE;
----------------
this isn't possible, as the item being consumed is the value of peek().
You could assert !reachedEnd() or just delete this.
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:227
+ float consume() override {
+ if (reachedEnd())
return DEFAULT_BOOST_SCORE;
----------------
and again here
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:341
+ float consume() override {
+ if (!reachedEnd())
+ --ItemsLeft;
----------------
if condition is always true
you could replace the if with an assertion, but the child will assert, so
better just to remove it.
replace if with assertion
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:348
+ llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+ OS << "(LIMIT items left " << ItemsLeft << *Child << ")";
+ return OS;
----------------
ISTM this should show the immutable state at the start of the query, rather
than the mutated state?
Or if you really think it's useful, both.
e.g. (LIMIT 5(3) <child>)
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:358
std::vector<std::pair<DocID, float>> consume(Iterator &It, size_t Limit) {
std::vector<std::pair<DocID, float>> Result;
----------------
I think you can remove the Limit parameter now, replacing it with an outer
Limit iterator
================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:362
It.advance(), ++Retrieved) {
DocID Document = It.peek();
+ Result.push_back(std::make_pair(Document, It.consume()));
----------------
you can inline this now
================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:267
TEST(DexIndexIterators, Limit) {
+ const PostingList L0 = {3, 6, 7, 20, 42, 100};
----------------
Is this testing the "limit" parameter to consume, or the limit iterator?
It shouldn't test both just because they happen to share a name :-)
(As noted above, I think you can remove the limit parameter)
https://reviews.llvm.org/D51029
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
