[clangd] NFC: Cleanup Dex Iterator comments and simplify tests

[kbobyrev]

Summary

Proposed changes:

• Cleanup comments in clangd/index/dex/Iterator.h: Vim's gq formatting added redundant spaces instead of newlines in few places
• Few comments in OrIterator are wrong
• Use EXPECT_TRUE(Condition) instead of EXPECT_THAT(Condition, true) (same with EXPECT_FALSE)
• Don't expose dump() method to the public by misplacing private:

This patch does not affect functionality.

[ioeric]

lgtm

[kbobyrev]

Something I initially forgot: fix misplaced private: so that dump() stays private as it was in the interface.

clang-tools-extra/trunk/clangd/index/dex/Iterator.h

 //===--- Iterator.h - Query Symbol Retrieval --------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Symbol index queries consist of specific requirements for the requested
 // 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.  Inverted index maps these tokens to the posting lists - sorted ( by
-// symbol quality) sequences of symbol IDs matching the token, e.g.  scope token
+// index. Inverted index maps these tokens to the posting lists - sorted (by
+// symbol quality) 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
 // declared in this namespace. Search queries are build from a set of
 // requirements which can be combined with each other forming the query trees.
 // The leafs of such trees are posting lists, and the nodes are operations on
-// these posting lists, e.g. intersection or union.  Efficient processing of
-// these multi-level queries is handled by Iterators.  Iterators advance through
+// these posting lists, e.g. intersection or union. Efficient processing of
+// these multi-level queries is handled by Iterators. Iterators advance through
 // all leaf posting lists producing the result of search query, which preserves
 // the sorted order of IDs. Having the resulting IDs sorted is important,
 // because it allows receiving a certain number of the most valuable items (e.g.
 // symbols with highest quality which was the sorting key in the first place)
 // without processing all items with requested properties (this might not be
 // computationally effective if search request is not very restrictive).
 //
 //===----------------------------------------------------------------------===//

clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp

void advanceTo(DocID ID) override {
     Index = std::lower_bound(Index, std::end(Documents), ID);
   }
 
   DocID peek() const override {
     assert(!reachedEnd() && "DocumentIterator can't call peek() at the end.");
     return *Index;
   }
 
+private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
     OS << '[';
     auto Separator = "";
     for (auto It = std::begin(Documents); It != std::end(Documents); ++It) {
       OS << Separator;
       if (It == Index)
         OS << '{' << *It << '}';
       else
         OS << *It;
       Separator = ", ";
     }
     OS << Separator;
     if (Index == std::end(Documents))
       OS << "{END}";
     else
       OS << "END";
     OS << ']';
     return OS;
   }
 
-private:
   PostingListRef Documents;
   PostingListRef::const_iterator Index;
 };
 
 /// Implements Iterator over the intersection of other iterators.
 ///
 /// AndIterator iterates through common items among all children. It becomes
 /// exhausted as soon as any child becomes exhausted. After each mutation, the
public:
   void advanceTo(DocID ID) override {
     assert(!reachedEnd() && "AndIterator can't call advanceTo() at the end.");
     Children.front()->advanceTo(ID);
     sync();
   }
 
   DocID peek() const override { return Children.front()->peek(); }
 
+private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
     OS << "(& ";
     auto Separator = "";
     for (const auto &Child : Children) {
       OS << Separator << *Child;
       Separator = " ";
     }
     OS << ')';
     return OS;
   }
 
-private:
   /// Restores class invariants: each child will point to the same element after
   /// sync.
   void sync() {
     ReachedEnd |= Children.front()->reachedEnd();
     if (ReachedEnd)
       return;
     auto SyncID = Children.front()->peek();
     // Indicates whether any child needs to be advanced to new SyncID.
return std::all_of(begin(Children), end(Children),
                        [](const std::unique_ptr<Iterator> &Child) {
                          return Child->reachedEnd();
                        });
   }
 
   /// Moves each child pointing to the smallest DocID to the next item.
   void advance() override {
     assert(!reachedEnd() &&
-           "OrIterator must have at least one child to advance().");
+           "OrIterator can't call advance() after it reached the end.");
     const auto SmallestID = peek();
     for (const auto &Child : Children)
       if (!Child->reachedEnd() && Child->peek() == SmallestID)
         Child->advance();
   }
 
   /// Advances each child to the next existing element with DocumentID >= ID.
   void advanceTo(DocID ID) override {
     assert(!reachedEnd() && "Can't advance iterator after it reached the end.");
     for (const auto &Child : Children)
       if (!Child->reachedEnd())
         Child->advanceTo(ID);
   }
 
   /// Returns the element under cursor of the child with smallest Child->peek()
   /// value.
   DocID peek() const override {
     assert(!reachedEnd() &&
-           "OrIterator must have at least one child to peek().");
+           "OrIterator can't peek() after it reached the end.");
     DocID Result = std::numeric_limits<DocID>::max();
 
     for (const auto &Child : Children)
       if (!Child->reachedEnd())
         Result = std::min(Result, Child->peek());
 
     return Result;
   }
 
+private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
     OS << "(| ";
     auto Separator = "";
     for (const auto &Child : Children) {
       OS << Separator << *Child;
       Separator = " ";
     }
     OS << ')';
     return OS;
   }
 
-private:
   // FIXME(kbobyrev): Would storing Children in min-heap be faster?
   std::vector<std::unique_ptr<Iterator>> Children;
 };
 
 /// TrueIterator handles PostingLists which contain all items of the index. It
 /// stores size of the virtual posting list, and all operations are performed
 /// in O(1).
 class TrueIterator : public Iterator {

clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp

 
 using ::testing::ElementsAre;
 
 TEST(DexIndexIterators, DocumentIterator) {
   const PostingList L = {4, 7, 8, 20, 42, 100};
   auto DocIterator = create(L);
 
   EXPECT_EQ(DocIterator->peek(), 4U);
-  EXPECT_EQ(DocIterator->reachedEnd(), false);
+  EXPECT_FALSE(DocIterator->reachedEnd());
 
   DocIterator->advance();
   EXPECT_EQ(DocIterator->peek(), 7U);
-  EXPECT_EQ(DocIterator->reachedEnd(), false);
+  EXPECT_FALSE(DocIterator->reachedEnd());
 
   DocIterator->advanceTo(20);
   EXPECT_EQ(DocIterator->peek(), 20U);
-  EXPECT_EQ(DocIterator->reachedEnd(), false);
+  EXPECT_FALSE(DocIterator->reachedEnd());
 
   DocIterator->advanceTo(65);
   EXPECT_EQ(DocIterator->peek(), 100U);
-  EXPECT_EQ(DocIterator->reachedEnd(), false);
+  EXPECT_FALSE(DocIterator->reachedEnd());
 
   DocIterator->advanceTo(420);
-  EXPECT_EQ(DocIterator->reachedEnd(), true);
+  EXPECT_TRUE(DocIterator->reachedEnd());
 }
 
 TEST(DexIndexIterators, AndWithEmpty) {
   const PostingList L0;
   const PostingList L1 = {0, 5, 7, 10, 42, 320, 9000};
 
   auto AndEmpty = createAnd(create(L0));
-  EXPECT_EQ(AndEmpty->reachedEnd(), true);
+  EXPECT_TRUE(AndEmpty->reachedEnd());
 
   auto AndWithEmpty = createAnd(create(L0), create(L1));
-  EXPECT_EQ(AndWithEmpty->reachedEnd(), true);
+  EXPECT_TRUE(AndWithEmpty->reachedEnd());
 
   EXPECT_THAT(consume(*AndWithEmpty), ElementsAre());
 }
 
 TEST(DexIndexIterators, AndTwoLists) {
   const PostingList L0 = {0, 5, 7, 10, 42, 320, 9000};
   const PostingList L1 = {0, 4, 7, 10, 30, 60, 320, 9000};
 
   auto And = createAnd(create(L1), create(L0));
 
-  EXPECT_EQ(And->reachedEnd(), false);
+  EXPECT_FALSE(And->reachedEnd());
   EXPECT_THAT(consume(*And), ElementsAre(0U, 7U, 10U, 320U, 9000U));
 
   And = createAnd(create(L0), create(L1));
 
   And->advanceTo(0);
   EXPECT_EQ(And->peek(), 0U);
   And->advanceTo(5);
   EXPECT_EQ(And->peek(), 7U);
TEST(DexIndexIterators, AndThreeLists) {
   const PostingList L2 = {1, 4, 7, 11, 30, 60, 320, 9000};
 
   auto And = createAnd(create(L0), create(L1), create(L2));
   EXPECT_EQ(And->peek(), 7U);
   And->advanceTo(300);
   EXPECT_EQ(And->peek(), 320U);
   And->advanceTo(100000);
 
-  EXPECT_EQ(And->reachedEnd(), true);
+  EXPECT_TRUE(And->reachedEnd());
 }
 
 TEST(DexIndexIterators, OrWithEmpty) {
   const PostingList L0;
   const PostingList L1 = {0, 5, 7, 10, 42, 320, 9000};
 
   auto OrEmpty = createOr(create(L0));
-  EXPECT_EQ(OrEmpty->reachedEnd(), true);
+  EXPECT_TRUE(OrEmpty->reachedEnd());
 
   auto OrWithEmpty = createOr(create(L0), create(L1));
-  EXPECT_EQ(OrWithEmpty->reachedEnd(), false);
+  EXPECT_FALSE(OrWithEmpty->reachedEnd());
 
   EXPECT_THAT(consume(*OrWithEmpty),
               ElementsAre(0U, 5U, 7U, 10U, 42U, 320U, 9000U));
 }
 
 TEST(DexIndexIterators, OrTwoLists) {
   const PostingList L0 = {0, 5, 7, 10, 42, 320, 9000};
   const PostingList L1 = {0, 4, 7, 10, 30, 60, 320, 9000};
 
   auto Or = createOr(create(L0), create(L1));
 
-  EXPECT_EQ(Or->reachedEnd(), false);
+  EXPECT_FALSE(Or->reachedEnd());
   EXPECT_EQ(Or->peek(), 0U);
   Or->advance();
   EXPECT_EQ(Or->peek(), 4U);
   Or->advance();
   EXPECT_EQ(Or->peek(), 5U);
   Or->advance();
   EXPECT_EQ(Or->peek(), 7U);
   Or->advance();
   EXPECT_EQ(Or->peek(), 10U);
   Or->advance();
   EXPECT_EQ(Or->peek(), 30U);
   Or->advanceTo(42);
   EXPECT_EQ(Or->peek(), 42U);
   Or->advanceTo(300);
   EXPECT_EQ(Or->peek(), 320U);
   Or->advanceTo(9000);
   EXPECT_EQ(Or->peek(), 9000U);
   Or->advanceTo(9001);
-  EXPECT_EQ(Or->reachedEnd(), true);
+  EXPECT_TRUE(Or->reachedEnd());
 
   Or = createOr(create(L0), create(L1));
 
   EXPECT_THAT(consume(*Or),
               ElementsAre(0U, 4U, 5U, 7U, 10U, 30U, 42U, 60U, 320U, 9000U));
 }
 
 TEST(DexIndexIterators, OrThreeLists) {
   const PostingList L0 = {0, 5, 7, 10, 42, 320, 9000};
   const PostingList L1 = {0, 4, 7, 10, 30, 60, 320, 9000};
   const PostingList L2 = {1, 4, 7, 11, 30, 60, 320, 9000};
 
   auto Or = createOr(create(L0), create(L1), create(L2));
 
-  EXPECT_EQ(Or->reachedEnd(), false);
+  EXPECT_FALSE(Or->reachedEnd());
   EXPECT_EQ(Or->peek(), 0U);
 
   Or->advance();
   EXPECT_EQ(Or->peek(), 1U);
 
   Or->advance();
   EXPECT_EQ(Or->peek(), 4U);
 
   Or->advanceTo(7);
 
   Or->advanceTo(59);
   EXPECT_EQ(Or->peek(), 60U);
 
   Or->advanceTo(9001);
-  EXPECT_EQ(Or->reachedEnd(), true);
+  EXPECT_TRUE(Or->reachedEnd());
 }
 
 // FIXME(kbobyrev): The testcase below is similar to what is expected in real
 // queries. It should be updated once new iterators (such as boosting, limiting,
 // etc iterators) appear. However, it is not exhaustive and it would be
 // beneficial to implement automatic generation of query trees for more
 // comprehensive testing.
 TEST(DexIndexIterators, QueryTree) {
TEST(DexIndexIterators, QueryTree) {
 
   // Root of the query tree: [1, 5]
   auto Root = createAnd(
       // Lower And Iterator: [1, 5, 9]
       createAnd(create(L0), create(L1)),
       // Lower Or Iterator: [0, 1, 5]
       createOr(create(L2), create(L3), create(L4)));
 
-  EXPECT_EQ(Root->reachedEnd(), false);
+  EXPECT_FALSE(Root->reachedEnd());
   EXPECT_EQ(Root->peek(), 1U);
   Root->advanceTo(0);
   // Advance multiple times. Shouldn't do anything.
   Root->advanceTo(1);
   Root->advanceTo(0);
   EXPECT_EQ(Root->peek(), 1U);
   Root->advance();
   EXPECT_EQ(Root->peek(), 5U);
   Root->advanceTo(5);
   EXPECT_EQ(Root->peek(), 5U);
   Root->advanceTo(9000);
-  EXPECT_EQ(Root->reachedEnd(), true);
+  EXPECT_TRUE(Root->reachedEnd());
 }
 
 TEST(DexIndexIterators, StringRepresentation) {
   const PostingList L0 = {4, 7, 8, 20, 42, 100};
   const PostingList L1 = {1, 3, 5, 8, 9};
   const PostingList L2 = {1, 5, 7, 9};
   const PostingList L3 = {0, 5};
   const PostingList L4 = {0, 1, 5};
TEST(DexIndexIterators, Limit) {
   EXPECT_THAT(consume(*DocIterator, 3), ElementsAre(4, 7, 8));
 
   DocIterator = create(L0);
   EXPECT_THAT(consume(*DocIterator, 0), ElementsAre());
 }
 
 TEST(DexIndexIterators, True) {
   auto TrueIterator = createTrue(0U);
-  EXPECT_THAT(TrueIterator->reachedEnd(), true);
+  EXPECT_TRUE(TrueIterator->reachedEnd());
   EXPECT_THAT(consume(*TrueIterator), ElementsAre());
 
   PostingList L0 = {1, 2, 5, 7};
   TrueIterator = createTrue(7U);
   EXPECT_THAT(TrueIterator->peek(), 0);
   auto AndIterator = createAnd(create(L0), move(TrueIterator));
-  EXPECT_THAT(AndIterator->reachedEnd(), false);
+  EXPECT_FALSE(AndIterator->reachedEnd());
   EXPECT_THAT(consume(*AndIterator), ElementsAre(1, 2, 5));
 }
 
 testing::Matcher<std::vector<Token>>
 trigramsAre(std::initializer_list<std::string> Trigrams) {
   std::vector<Token> Tokens;
   for (const auto &Symbols : Trigrams) {
     Tokens.push_back(Token(Token::Kind::Trigram, Symbols));