[PATCH] D51528: [NFC] Cleanup Dex

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341190: [NFC] Cleanup Dex (authored by omtcyfz, committed by 
).
Herald added subscribers: llvm-commits, ilya-biryukov.

Changed prior to commit:
  https://reviews.llvm.org/D51528?vs=163485&id=163489#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51528

Files:
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
  clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
  clang-tools-extra/trunk/clangd/index/dex/Trigram.cpp

Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
@@ -41,7 +41,7 @@
 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> Symbols);
+  void build(std::shared_ptr> Syms);
 
   /// \brief Build index from a symbol slab.
   static std::unique_ptr build(SymbolSlab Slab);
Index: clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.cpp
@@ -30,23 +30,26 @@
 
   /// 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(); }
 
@@ -84,7 +87,7 @@
 public:
   AndIterator(std::vector> 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
@@ -105,22 +108,22 @@
 
   /// 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 &Child) {
@@ -192,7 +195,7 @@
 public:
   OrIterator(std::vector> 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.
@@ -205,27 +208,25 @@
 
   /// 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.");
+   

[PATCH] D51528: [NFC] Cleanup Dex

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 163485.
kbobyrev marked 2 inline comments as done.

https://reviews.llvm.org/D51528

Files:
  clang-tools-extra/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/clangd/index/dex/DexIndex.h
  clang-tools-extra/clangd/index/dex/Iterator.cpp
  clang-tools-extra/clangd/index/dex/Trigram.cpp

Index: clang-tools-extra/clangd/index/dex/Trigram.cpp
===
--- clang-tools-extra/clangd/index/dex/Trigram.cpp
+++ clang-tools-extra/clangd/index/dex/Trigram.cpp
@@ -87,10 +87,10 @@
 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]}});
@@ -113,8 +113,8 @@
   // 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();
Index: clang-tools-extra/clangd/index/dex/Iterator.cpp
===
--- clang-tools-extra/clangd/index/dex/Iterator.cpp
+++ clang-tools-extra/clangd/index/dex/Iterator.cpp
@@ -30,23 +30,26 @@
 
   /// 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(); }
 
@@ -84,7 +87,7 @@
 public:
   AndIterator(std::vector> 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
@@ -105,22 +108,22 @@
 
   /// 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 &Child) {
@@ -192,7 +195,7 @@
 public:
   OrIterator(std::vector> 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.
@@ -205,27 +208,25 @@
 
   /// 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();
   }
 
   

[PATCH] D51528: [NFC] Cleanup Dex

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:33
   std::vector Result = generateIdentifierTrigrams(Sym.Name);
-  Result.push_back(Token(Token::Kind::Scope, Sym.Scope));
+  Result.emplace_back(Token(Token::Kind::Scope, Sym.Scope));
   return Result;

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



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:395
   for (; !It.reachedEnd(); It.advance())
-Result.push_back(std::make_pair(It.peek(), It.consume()));
+Result.emplace_back(std::make_pair(It.peek(), It.consume()));
   return Result;

No need for `make_pair` here?


https://reviews.llvm.org/D51528



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51528: [NFC] Cleanup Dex

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

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


https://reviews.llvm.org/D51528



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51528: [NFC] Cleanup Dex

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added reviewers: ioeric, sammccall.
kbobyrev added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, jkorous.

- 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


https://reviews.llvm.org/D51528

Files:
  clang-tools-extra/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/clangd/index/dex/DexIndex.h
  clang-tools-extra/clangd/index/dex/Iterator.cpp
  clang-tools-extra/clangd/index/dex/Trigram.cpp

Index: clang-tools-extra/clangd/index/dex/Trigram.cpp
===
--- clang-tools-extra/clangd/index/dex/Trigram.cpp
+++ clang-tools-extra/clangd/index/dex/Trigram.cpp
@@ -87,10 +87,10 @@
 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]}});
@@ -113,8 +113,8 @@
   // 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();
Index: clang-tools-extra/clangd/index/dex/Iterator.cpp
===
--- clang-tools-extra/clangd/index/dex/Iterator.cpp
+++ clang-tools-extra/clangd/index/dex/Iterator.cpp
@@ -30,23 +30,26 @@
 
   /// 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(); }
 
@@ -84,7 +87,7 @@
 public:
   AndIterator(std::vector> 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
@@ -105,22 +108,22 @@
 
   /// 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 &Child) {
@@ -192,7 +195,7 @@
 public:
   OrIterator(std::vector> 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.
@@ -205,27 +208,25 @@
 
   /// Moves each child pointing to the smallest DocID to the next item.
   void advan