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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments!



================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:129
+      cxxForRangeStmt(
+          hasRangeInit(declRefExpr(supportedContainerTypesMatcher())),
+          HasInterestedLoopBody, InInterestedCompoundStmt)
----------------
alexfh wrote:
> 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?
Nice brainstorm, added a FIXME.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:152
+
+  const Stmt * LoopStmt = nullptr;
+  if (ForLoop)
----------------
alexfh wrote:
> const Stmt *LoopStmt = ForLoop ? ForLoop : RangeLoop;
We can't do it because `ForLoop` and `RangeLoop` are different types.


================
Comment at: clang-tidy/performance/InefficientVectorOperationCheck.cpp:198
+        Context->getLangOpts());
+    ReserveStmt = (VectorVarName + ".reserve(" + LoopEndSource + ");\n").str();
+  }
----------------
alexfh wrote:
> 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.
This case should not happen, because we only find the push_back loop before which there are no usages (defined as DeclRefExpr that refers to vector`v`) of the vector `v`.


https://reviews.llvm.org/D32436