[PATCH] D31757: [clang-tidy] Add a clang-tidy check for possible inefficient vector operations

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Awesome, thanks! A few late comments inline.



================
Comment at: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp:56
+void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) {
+  const auto VectorDecl = cxxRecordDecl(hasName("::std::vector"));
+  const auto VectorDefaultConstructorCall = cxxConstructExpr(
----------------
It might make sense to make the list of types configurable to support custom vector-like types.


================
Comment at: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp:65
+      cxxMemberCallExpr(
+          callee(cxxMethodDecl(hasName("push_back"))), on(hasType(VectorDecl)),
+          onImplicitObjectArgument(declRefExpr(to(VectorVarDecl))))
----------------
Loops with `emplace_back` would suffer from the same issue.


================
Comment at: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp:131
+
+  llvm::StringRef LoopEndSource = clang::Lexer::getSourceText(
+      CharSourceRange::getTokenRange(LoopEndExpr->getSourceRange()), SM,
----------------
No need to namespace-qualify StringRef and Lexer.


================
Comment at: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp:133
+      CharSourceRange::getTokenRange(LoopEndExpr->getSourceRange()), SM,
+      clang::LangOptions());
+  llvm::StringRef VectorVarName = clang::Lexer::getSourceText(
----------------
Actual LangOptions should be used instead of a default-constructed instance. Same below.


================
Comment at: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp:143
+       "'push_back' is called inside a loop; "
+       "consider pre-allocating the vector capacity before the loop.")
+      << FixItHint::CreateInsertion(ForLoop->getLocStart(), ReserveStmt);
----------------
Please remove the trailing period.


Repository:
  rL LLVM

https://reviews.llvm.org/D31757