[clang-tidy] fix modernize-loop-convert to retain needed array-like operator[]

[poelmanc]

Summary

modernize-loop-convert handles array-like objects like vectors fairly well, but strips slightly too much information from the iteration expression by converting:

Vector<Vector<int>> X;
for (int J = 0; J < X[5].size(); ++J)
  copyArg(X[5][J]);

to

Vector<Vector<int>> X;
for (int J : X) // should be for (int J : X[5]) 
  copyArg(J);

The [5] is a call to operator[] and gets stripped by LoopConvertCheck::getContainerString. This patch fixes that and adds several test cases.

[njames93]

LGTM, Can you fix the formatting as well.

clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp

51

Not essential, but how would this handle iterating the lowest dimension.

for (int J = 0; J < Y[3][4].size(); ++J)

Though I may have a feeling the check won't even try to convert that. If so disregard this comment.

[poelmanc]

Fix formatting, add suggested test case (which works.)

[poelmanc]

@njames93 Thanks for the review and for accepting this revision. I lack llvm-project commit access so if it's good to go I would greatly appreciate it if you or someone could push this whenever you have have a chance. Thanks!

[njames93]

In D95771#2540598, @poelmanc wrote:

@njames93 Thanks for the review and for accepting this revision. I lack llvm-project commit access so if it's good to go I would greatly appreciate it if you or someone could push this whenever you have have a chance. Thanks!

Should this be committed using poelmanc <[e-mail]>?

[poelmanc]

In D95771#2547102, @njames93 wrote:

Should this be committed using poelmanc <[e-mail]>?

That or Conrad Poelman <[e-mail]> would be great, thanks.

[njames93]

In D95771#2547160, @poelmanc wrote:

In D95771#2547102, @njames93 wrote:

Should this be committed using poelmanc <[e-mail]>?

That or Conrad Poelman <[e-mail]> would be great, thanks.

I committed using the email provided but its not attributed you on github. It's attributed you here though. is that email not linked to your github account?

[poelmanc]

In D95771#2547366, @njames93 wrote:

In D95771#2547160, @poelmanc wrote:

In D95771#2547102, @njames93 wrote:

Should this be committed using poelmanc <[e-mail]>?

That or Conrad Poelman <[e-mail]> would be great, thanks.

I committed using the email provided but its not attributed you on github. It's attributed you here though. is that email not linked to your github account?

Thanks for the commit, and thanks for the heads-up! I've now added the address in my github account.

[njames93]

In D95771#2547467, @poelmanc wrote:

Thanks for the commit, and thanks for the heads-up! I've now added the address in my github account.

Its updated now and the commit is linked to your account.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

 StringRef LoopConvertCheck::getContainerString(ASTContext *Context,
                                                const ForStmt *Loop,
                                                const Expr *ContainerExpr) {
   StringRef ContainerString;
   ContainerExpr = ContainerExpr->IgnoreParenImpCasts();
   if (isa<CXXThisExpr>(ContainerExpr)) {
     ContainerString = "this";
   } else {
-    // For CXXOperatorCallExpr (e.g. vector_ptr->size()), its first argument is
-    // the class object (vector_ptr) we are targeting.
+    // For CXXOperatorCallExpr such as vector_ptr->size() we want the class
+    // object vector_ptr, but for vector[2] we need the whole expression.
     if (const auto* E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr))
+      if (E->getOperator() != OO_Subscript)
         ContainerExpr = E->getArg(0);
     ContainerString =
         getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
                            ContainerExpr->getSourceRange());
   }
 
   return ContainerString;
 }
 

clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp

+// RUN: %check_clang_tidy %s modernize-loop-convert %t
+
+template <class T>
+struct Vector {
+  using iterator = T*;
+  unsigned size() const;
+  const T &operator[](int) const;
+  T &operator[](int);
+  T *begin();
+  T *end();
+  const T *begin() const;
+  const T *end() const;
+};
+
+template <typename T>
+void copyArg(T);
+
+class TestArrayOfVector {
+  Vector<int> W[2];
+
+  void foo() const {
+    for (int I = 0; I < W[0].size(); ++I) {
+      if (W[0][I])
+        copyArg(W[0][I]);
+    }
+    // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop
+    // CHECK-FIXES: for (int I : W[0]) {
+    // CHECK-FIXES-NEXT: if (I)
+    // CHECK-FIXES-NEXT: copyArg(I);
+  }
+};
+
+class TestVectorOfVector {
+  Vector<Vector<int>> X;
+
+  void foo() const {
+    for (int J = 0; J < X[0].size(); ++J) {
+      if (X[0][J])
+        copyArg(X[0][J]);
+    }
+    // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop
+    // CHECK-FIXES: for (int J : X[0]) {
+    // CHECK-FIXES-NEXT: if (J)
+    // CHECK-FIXES-NEXT: copyArg(J);
+  }
+};
+
+void testVectorOfVectorOfVector() {
+  Vector<Vector<Vector<int>>> Y;
+  for (int J = 0; J < Y[3].size(); ++J) {
+    if (Y[3][J][7])
+      copyArg(Y[3][J][8]);
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop
+  // CHECK-FIXES: for (auto & J : Y[3]) {
+  // CHECK-FIXES-NEXT: if (J[7])
+  // CHECK-FIXES-NEXT: copyArg(J[8]);
+
+  for (int J = 0; J < Y[3][4].size(); ++J) {
+    if (Y[3][4][J])
+      copyArg(Y[3][4][J]);
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop
+  // CHECK-FIXES: for (int J : Y[3][4]) {
+  // CHECK-FIXES-NEXT: if (J)
+  // CHECK-FIXES-NEXT: copyArg(J);
+}
+
+void testVectorOfVectorIterator() {
+  Vector<Vector<int>> Z;
+  for (Vector<int>::iterator it = Z[4].begin(); it != Z[4].end();  ++it) {
+    if (*it)
+      copyArg(*it);
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop
+  // CHECK-FIXES: for (int & it : Z[4]) {
+  // CHECK-FIXES-NEXT: if (it)
+  // CHECK-FIXES-NEXT: copyArg(it);
+}