[PATCH] D52789: [clangd] Dex: FALSE iterator, peephole optimizations, fix AND bug

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM and a few NITs.



================
Comment at: clangd/index/dex/Iterator.cpp:376
+    }
+    default:
+      RealChildren.push_back(std::move(Child));
----------------
Maybe replace with `case Kind::Other` to make sure compiler gives a warning to update the switch when new kinds are added?
Same for other switches on kind


================
Comment at: clangd/index/dex/Iterator.cpp:378
+      RealChildren.push_back(std::move(Child));
+    }
+  switch (RealChildren.size()) {
----------------
Add braces for `for` statement to make sure there's a closing brace at the right indentation?


================
Comment at: clangd/index/dex/Iterator.cpp:385
+  default:
+    return llvm::make_unique<AndIterator>(move(RealChildren));
+  }
----------------
Maybe use the qualified `std::move`  for consistency with the previous line?


================
Comment at: clangd/index/dex/Iterator.cpp:407
+      RealChildren.push_back(std::move(Child));
+    }
+  switch (RealChildren.size()) {
----------------
Again, `for () switch` pattern ends up putting the closing brace at the wrong indent level. Consider adding the braces around for the loop body.


================
Comment at: clangd/index/dex/Iterator.cpp:414
+  default:
+    return llvm::make_unique<OrIterator>(move(RealChildren));
+  }
----------------
Maybe use the qualified `std::move` for consistency with the previous line?




================
Comment at: clangd/index/dex/Iterator.h:109
 private:
+  Kind MyKind;
   virtual llvm::raw_ostream &dump(llvm::raw_ostream &OS) const = 0;
----------------
Maybe put data members after functions to follow the pattern we have in most (all?) other places in clangd?



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52789