[NFC] Cleanup Dex

[kbobyrev]

Summary

• Use consistent assertion messages in iterators implementations
• Silence a bunch of clang-tidy warnings: use emplace_back instead of push_back where possible, make sure arguments have the same name in header and implementation file, use for loop over ranges where possible

[kbobyrev]

The next step would be to move Dex to index/ and slightly simplify file structure, but I guess I should consider doing that after D51422 lands, otherwise it's a lot of effort to keep rebasing everything on top of different patches.

[ioeric]

clang-tools-extra/clangd/index/dex/DexIndex.cpp

33 ↗

(On Diff #163473)

Do you still need to spell out the constructor with emplace_back?

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

395 ↗

(On Diff #163473)

No need for make_pair here?

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

 // index so that it can be loaded from the disk. Since static index is not
 // changed frequently, it's safe to assume that it has to be built only once
 // (when the clangd process starts). Therefore, it can be easier to store built
 // index on disk and then load it if available.
 class DexIndex : public SymbolIndex {
 public:
   /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain
   /// accessible as long as `Symbols` is kept alive.
-  void build(std::shared_ptr<std::vector<const Symbol *>> Symbols);
+  void build(std::shared_ptr<std::vector<const Symbol *>> Syms);
 
   /// \brief Build index from a symbol slab.
   static std::unique_ptr<SymbolIndex> build(SymbolSlab Slab);
 
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
             llvm::function_ref<void(const Symbol &)> Callback) const override;
 

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

 // types of tokens such as symbol type (if applicable).
 // Returns the tokens which are given symbols's characteristics. For example,
 // trigrams and scopes.
 // FIXME(kbobyrev): Support more token types:
 // * Path proximity
 // * Types
 std::vector<Token> generateSearchTokens(const Symbol &Sym) {
   std::vector<Token> Result = generateIdentifierTrigrams(Sym.Name);
-  Result.push_back(Token(Token::Kind::Scope, Sym.Scope));
+  Result.emplace_back(Token::Kind::Scope, Sym.Scope);
   return Result;
 }
 
 } // namespace
 
 void DexIndex::build(std::shared_ptr<std::vector<const Symbol *>> Syms) {
   llvm::DenseMap<SymbolID, const Symbol *> TempLookupTable;
   llvm::DenseMap<const Symbol *, float> TempSymbolQuality;

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

 public:
   DocumentIterator(PostingListRef Documents)
       : Documents(Documents), Index(std::begin(Documents)) {}
 
   bool reachedEnd() const override { return Index == std::end(Documents); }
 
   /// Advances cursor to the next item.
   void advance() override {
-    assert(!reachedEnd() && "DocumentIterator can't advance at the end.");
+    assert(!reachedEnd() && "DOCUMENT iterator can't advance() at the end.");
     ++Index;
   }
 
   /// Applies binary search to advance cursor to the next item with DocID equal
   /// or higher than the given one.
   void advanceTo(DocID ID) override {
-    assert(!reachedEnd() && "DocumentIterator can't advance at the end.");
+    assert(!reachedEnd() && "DOCUMENT iterator can't advance() at the end.");
     Index = std::lower_bound(Index, std::end(Documents), ID);
   }
 
   DocID peek() const override {
-    assert(!reachedEnd() && "DocumentIterator can't call peek() at the end.");
+    assert(!reachedEnd() && "DOCUMENT iterator can't peek() at the end.");
     return *Index;
   }
 
-  float consume() override { return DEFAULT_BOOST_SCORE; }
+  float consume() override {
+    assert(!reachedEnd() && "DOCUMENT iterator can't consume() at the end.");
+    return DEFAULT_BOOST_SCORE;
+  }
 
   size_t estimateSize() const override { return Documents.size(); }
 
 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) {

 ///
 /// AndIterator iterates through common items among all children. It becomes
 /// exhausted as soon as any child becomes exhausted. After each mutation, the
 /// iterator restores the invariant: all children must point to the same item.
 class AndIterator : public Iterator {
 public:
   AndIterator(std::vector<std::unique_ptr<Iterator>> AllChildren)
       : Children(std::move(AllChildren)) {
-    assert(!Children.empty() && "AndIterator should have at least one child.");
+    assert(!Children.empty() && "AND iterator should have at least one child.");
     // Establish invariants.
     sync();
     // When children are sorted by the estimateSize(), sync() calls are more
     // effective. Each sync() starts with the first child and makes sure all
     // children point to the same element. If any child is "above" the previous
     // ones, the algorithm resets and and advances the children to the next
     // highest element starting from the front. When child iterators in the
     // beginning have smaller estimated size, the sync() will have less restarts
     // and become more effective.
     std::sort(begin(Children), end(Children),
               [](const std::unique_ptr<Iterator> &LHS,
                  const std::unique_ptr<Iterator> &RHS) {
                 return LHS->estimateSize() < RHS->estimateSize();
               });
   }
 
   bool reachedEnd() const override { return ReachedEnd; }
 
   /// Advances all children to the next common item.
   void advance() override {
-    assert(!reachedEnd() && "AndIterator can't call advance() at the end.");
+    assert(!reachedEnd() && "AND iterator can't advance() at the end.");
     Children.front()->advance();
     sync();
   }
 
   /// Advances all children to the next common item with DocumentID >= ID.
   void advanceTo(DocID ID) override {
-    assert(!reachedEnd() && "AndIterator can't call advanceTo() at the end.");
+    assert(!reachedEnd() && "AND iterator can't advanceTo() at the end.");
     Children.front()->advanceTo(ID);
     sync();
   }
 
   DocID peek() const override { return Children.front()->peek(); }
 
   float consume() override {
-    assert(!reachedEnd() && "AndIterator can't consume() at the end.");
+    assert(!reachedEnd() && "AND iterator can't consume() at the end.");
     return std::accumulate(
         begin(Children), end(Children), DEFAULT_BOOST_SCORE,
         [&](float Current, const std::unique_ptr<Iterator> &Child) {
           return Current * Child->consume();
         });
   }
 
   size_t estimateSize() const override {

 /// OrIterator iterates through all items which can be pointed to by at least
 /// one child. To preserve the sorted order, this iterator always advances the
 /// child with smallest Child->peek() value. OrIterator becomes exhausted as
 /// soon as all of its children are exhausted.
 class OrIterator : public Iterator {
 public:
   OrIterator(std::vector<std::unique_ptr<Iterator>> AllChildren)
       : Children(std::move(AllChildren)) {
-    assert(Children.size() > 0 && "Or Iterator must have at least one child.");
+    assert(Children.size() > 0 && "OR iterator must have at least one child.");
   }
 
   /// Returns true if all children are exhausted.
   bool reachedEnd() const override {
     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 can't call advance() after it reached the end.");
+    assert(!reachedEnd() && "OR iterator can't advance() at 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.");
+    assert(!reachedEnd() && "OR iterator can't advanceTo() at 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 can't peek() after it reached the end.");
+    assert(!reachedEnd() && "OR iterator can't peek() at 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;
   }
 
   // Returns the maximum boosting score among all Children when iterator is not
   // exhausted and points to the given ID, DEFAULT_BOOST_SCORE otherwise.
   float consume() override {
-    assert(!reachedEnd() &&
-           "OrIterator can't consume() after it reached the end.");
+    assert(!reachedEnd() && "OR iterator can't consume() at the end.");
     const DocID ID = peek();
     return std::accumulate(
         begin(Children), end(Children), DEFAULT_BOOST_SCORE,
         [&](float Boost, const std::unique_ptr<Iterator> &Child) {
           return (!Child->reachedEnd() && Child->peek() == ID)
                      ? std::max(Boost, Child->consume())
                      : Boost;
         });

 /// in O(1).
 class TrueIterator : public Iterator {
 public:
   TrueIterator(DocID Size) : Size(Size) {}
 
   bool reachedEnd() const override { return Index >= Size; }
 
   void advance() override {
-    assert(!reachedEnd() && "Can't advance iterator after it reached the end.");
+    assert(!reachedEnd() && "TRUE iterator can't advance() at the end.");
     ++Index;
   }
 
   void advanceTo(DocID ID) override {
-    assert(!reachedEnd() && "Can't advance iterator after it reached the end.");
+    assert(!reachedEnd() && "TRUE iterator can't advanceTo() at the end.");
     Index = std::min(ID, Size);
   }
 
   DocID peek() const override {
-    assert(!reachedEnd() && "TrueIterator can't call peek() at the end.");
+    assert(!reachedEnd() && "TRUE iterator can't peek() at the end.");
     return Index;
   }
 
-  float consume() override { return DEFAULT_BOOST_SCORE; }
+  float consume() override {
+    assert(!reachedEnd() && "TRUE iterator can't consume() at the end.");
+    return DEFAULT_BOOST_SCORE;
+  }
 
   size_t estimateSize() const override { return Size; }
 
 private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
     OS << "(TRUE {" << Index << "} out of " << Size << ")";
     return OS;
   }
public:
 
   void advanceTo(DocID ID) override { Child->advanceTo(ID); }
 
   DocID peek() const override { return Child->peek(); }
 
   /// Decreases the limit in case the element consumed at top of the query tree
   /// comes from the underlying iterator.
   float consume() override {
-    assert(!reachedEnd() && "LimitIterator can't consume at the end.");
+    assert(!reachedEnd() && "LimitIterator can't consume() at the end.");
     --ItemsLeft;
     return Child->consume();
   }
 
   size_t estimateSize() const override {
     return std::min(Child->estimateSize(), Limit);
   }
 
 private:
   llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
     OS << "(LIMIT " << Limit << '(' << ItemsLeft << ") " << *Child << ')';
     return OS;
   }
 
   std::unique_ptr<Iterator> Child;
   size_t Limit;
   size_t ItemsLeft;
 };
 
 } // end namespace
 
 std::vector<std::pair<DocID, float>> consume(Iterator &It) {
   std::vector<std::pair<DocID, float>> Result;
   for (; !It.reachedEnd(); It.advance())
-    Result.push_back(std::make_pair(It.peek(), It.consume()));
+    Result.emplace_back(It.peek(), It.consume());
   return Result;
 }
 
 std::unique_ptr<Iterator> create(PostingListRef Documents) {
   return llvm::make_unique<DocumentIterator>(Documents);
 }
 
 std::unique_ptr<Iterator>

 }
 
 std::unique_ptr<Iterator> createBoost(std::unique_ptr<Iterator> Child,
                                       float Factor) {
   return llvm::make_unique<BoostIterator>(move(Child), Factor);
 }
 
 std::unique_ptr<Iterator> createLimit(std::unique_ptr<Iterator> Child,
-                                      size_t Size) {
-  return llvm::make_unique<LimitIterator>(move(Child), Size);
+                                      size_t Limit) {
+  return llvm::make_unique<LimitIterator>(move(Child), Limit);
 }
 
 } // namespace dex
 } // namespace clangd
 } // namespace clang

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

std::vector<Token> generateIdentifierTrigrams(llvm::StringRef Identifier) {
 
   // Iterate through valid seqneces of three characters Fuzzy Matcher can
   // process.
   for (size_t I = 0; I < LowercaseIdentifier.size(); ++I) {
     // Skip delimiters.
     if (Roles[I] != Head && Roles[I] != Tail)
       continue;
     for (const unsigned J : Next[I]) {
-      if (!J)
+      if (J == 0)
         continue;
       for (const unsigned K : Next[J]) {
-        if (!K)
+        if (K == 0)
           continue;
         add({{LowercaseIdentifier[I], LowercaseIdentifier[J],
               LowercaseIdentifier[K]}});
       }
     }
   }
 
   std::vector<Token> Result;
   for (const auto &Trigram : UniqueTrigrams)
     Result.push_back(Trigram);
 
   return Result;
 }
 
 std::vector<Token> generateQueryTrigrams(llvm::StringRef Query) {
   // Apply fuzzy matching text segmentation.
   std::vector<CharRole> Roles(Query.size());
   calculateRoles(Query, llvm::makeMutableArrayRef(Roles.data(), Query.size()));
 
   // Additional pass is necessary to count valid identifier characters.
   // Depending on that, this function might return incomplete trigram.
   unsigned ValidSymbolsCount = 0;
-  for (size_t I = 0; I < Roles.size(); ++I)
-    if (Roles[I] == Head || Roles[I] == Tail)
+  for (const auto Role : Roles)
+    if (Role == Head || Role == Tail)
       ++ValidSymbolsCount;
 
   std::string LowercaseQuery = Query.lower();
 
   DenseSet<Token> UniqueTrigrams;
 
   // If the number of symbols which can form fuzzy matching trigram is not
   // sufficient, generate a single incomplete trigram for query.