[clang-tools-extra] 5229edd - [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[]

[Nathan James via cfe-commits]

Author: poelmanc
Date: 2021-02-07T16:36:34Z
New Revision: 5229edd66742aeed407f774d77d437fa80347ad1

URL: https://github.com/llvm/llvm-project/commit/5229edd66742aeed407f774d77d437fa80347ad1
DIFF: https://github.com/llvm/llvm-project/commit/5229edd66742aeed407f774d77d437fa80347ad1.diff

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

`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.

Reviewed By: njames93

Differential Revision: https://reviews.llvm.org/D95771

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

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index e6b7c1f6025e..ec75be9c5f53 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -712,10 +712,11 @@ StringRef LoopConvertCheck::getContainerString(ASTContext *Context,
   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))
-      ContainerExpr = E->getArg(0);
+      if (E->getOperator() != OO_Subscript)
+        ContainerExpr = E->getArg(0);
     ContainerString =
         getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
                            ContainerExpr->getSourceRange());

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
new file mode 100644
index 000000000000..bffb7f560559
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
@@ -0,0 +1,79 @@
+// 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);
+}