[PATCH] D32436: [clang-tidy] Support detecting for-range loop in inefficient-vector-operation check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Cool! A few nits.



================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:56
+ast_matchers::internal::Matcher<Expr> supportedContainerTypesMatcher() {
+  const auto types = cxxRecordDecl(hasAnyName(
+      "::std::vector", "::std::set", "::std::unordered_set", "::std::map",
----------------
I'd just inline the expression and remove the local variable, since it doesn't seem to bring any benefits.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:103
+  // Matchers for the loop whose body has only 1 push_back calling statement.
+  const auto HasInterestedLoopBody = hasBody(anyOf(
+      compoundStmt(statementCountIs(1), has(PushBackCall)), PushBackCall));
----------------
s/Interested/Interesting/


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:129
+      cxxForRangeStmt(
+          hasRangeInit(declRefExpr(supportedContainerTypesMatcher())),
+          HasInterestedLoopBody, InInterestedCompoundStmt)
----------------
It might be valuable to support more complex expressions here, though it would require more involved fixes, e.g.

  for (const auto& e : *source)
    v.push_back(e);

->

  v.reserve(source->size());
  for (const auto& e : *source)
    v.push_back(e);

or even like this:

  for (const auto& e : *some(complex)->container().expression())
    v.push_back(e);

->

  const auto& source_container = *some(complex)->container().expression();
  v.reserve(source_container.size());
  for (const auto& e : source_container)
    v.push_back(e);

Maybe leave a FIXME for this?


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:152
+
+  const Stmt * LoopStmt = nullptr;
+  if (ForLoop)
----------------
const Stmt *LoopStmt = ForLoop ? ForLoop : RangeLoop;


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:185
+    // `for (range-declarator: range-expression)`.
+    llvm::StringRef RangeInitExpName = clang::Lexer::getSourceText(
+        CharSourceRange::getTokenRange(
----------------
No need to qualify `StringRef` and `Lexer`. Here and a couple of other instances in this function.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:198
+        Context->getLangOpts());
+    ReserveStmt = (VectorVarName + ".reserve(" + LoopEndSource + ");\n").str();
+  }
----------------
Does the check handle destination vector not being empty before the push_back loop? We'd need to reserve `destination.size() + source.size()` items in this case.

If this is not handled yet, please add a FIXME.


https://reviews.llvm.org/D32436