Clang-tidy invalid suggestion: modernize-loop-convert

[joker-eph]

With clang-tidy built at current head and applying it on MLIR:

$ clang-tidy -p build mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp --checks=-*,modernize-loop-convert -fix -fix-errors

diff --git a/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp b/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp
index 68ef51992efe..e5c3710971d4 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp
@@ -307,8 +307,7 @@ LogicalResult bufferization::bufferizeOp(Operation *op,
   // Bufferize all ops.
   BufferizationRewriter rewriter(op->getContext(), erasedOps, toBufferOps,
                                  worklist, options, statistics);
-  for (unsigned i = 0; i < worklist.size(); ++i) {
-    Operation *nextOp = worklist[i];
+  for (auto nextOp : worklist) {
     // Skip ops that were erased.
     if (erasedOps.contains(nextOp))
       continue;

worklist is a vector: however there is a callback that will push_back() to the worklist , which can cause reallocation of the vector and iterator invalidation here.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Mehdi Amini (joker-eph)

With clang-tidy built at current head and applying it on MLIR:

$ clang-tidy -p build mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp --checks=-*,modernize-loop-convert -fix -fix-errors

diff --git a/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp b/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp
index 68ef51992efe..e5c3710971d4 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/Bufferize.cpp
@@ -307,8 +307,7 @@ LogicalResult bufferization::bufferizeOp(Operation *op,
   // Bufferize all ops.
   BufferizationRewriter rewriter(op->getContext(), erasedOps, toBufferOps,
                                  worklist, options, statistics);
-  for (unsigned i = 0; i < worklist.size(); ++i) {
-    Operation *nextOp = worklist[i];
+  for (auto nextOp : worklist) {
     // Skip ops that were erased.
     if (erasedOps.contains(nextOp))
       continue;

worklist is a vector: however there is a callback that will push_back() to the worklist , which can cause reallocation of the vector and iterator invalidation here.

[carlosgalvezp]

which can cause reallocation of the vector and iterator invalidation here.

How is that avoided with the code before applying the fix?

[joker-eph]

We don't iterate using the vector iterator, we have a loop index.
The vector growing isn't an issue when you use an index.

[HerrCai0907]

I think it is hard to avoid this false positive because clang-tidy does not know the operation of worklist which is hidden in other method.
@carlosgalvezp do you think it is a good idea to add option that can suppress warning when loop variable is class field.

[joker-eph]

I don't think that " loop variable is class field" is the right discrimant: it could be a local variable updated by a lambda:

vector<int> worklist(1, 1);
auto enqueue = [&] (int e) { wordlist.push_back(e); };
...

[HerrCai0907]

The fully support need to borrow checker to make sure vector are not modified outside. IMO it is impossible.
clang-tidy needs to find the balance between false positive and false negative.