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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D95771#2547366 , @njames93 wrote:

> In D95771#2547160 , @poelmanc wrote:
>
>> In D95771#2547102 , @njames93 wrote:
>>
>>> Should this be committed using `poelmanc `?
>>
>> That or `Conrad Poelman ` 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D95771#2547160 , @poelmanc wrote:

> In D95771#2547102 , @njames93 wrote:
>
>> Should this be committed using `poelmanc `?
>
> That or `Conrad Poelman ` 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5229edd66742: [clang-tidy] fix modernize-loop-convert to 
retain needed array-like operator[] (authored by poelmanc, committed by 
njames93).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

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

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
===
--- /dev/null
+++ 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 
+struct Vector {
+  using iterator = T*;
+  unsigned size() const;
+  const T [](int) const;
+  T [](int);
+  T *begin();
+  T *end();
+  const T *begin() const;
+  const T *end() const;
+};
+
+template 
+void copyArg(T);
+
+class TestArrayOfVector {
+  Vector 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> 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>> 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> Z;
+  for (Vector::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);
+}
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -712,10 +712,11 @@
   if (isa(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(ContainerExpr))
-  ContainerExpr = E->getArg(0);
+  if (E->getOperator() != OO_Subscript)
+ContainerExpr = E->getArg(0);
 ContainerString =
 getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
ContainerExpr->getSourceRange());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D95771#2547102 , @njames93 wrote:

> Should this be committed using `poelmanc `?

That or `Conrad Poelman ` would be great, thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.
Herald added a subscriber: nullptr.cpp.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

@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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 320704.
poelmanc marked an inline comment as done.
poelmanc added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

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

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
===
--- /dev/null
+++ 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 
+struct Vector {
+  using iterator = T*;
+  unsigned size() const;
+  const T [](int) const;
+  T [](int);
+  T *begin();
+  T *end();
+  const T *begin() const;
+  const T *end() const;
+};
+
+template 
+void copyArg(T);
+
+class TestArrayOfVector {
+  Vector 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> 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>> 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> Z;
+  for (Vector::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);
+}
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -722,10 +722,11 @@
   if (isa(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(ContainerExpr))
-  ContainerExpr = E->getArg(0);
+  if (E->getOperator() != OO_Subscript)
+ContainerExpr = E->getArg(0);
 ContainerString =
 getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
ContainerExpr->getSourceRange());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 edited reviewers, added: aaron.ballman, alexfh, njames93; removed: 
sammccall, hokein.
njames93 accepted this revision.
njames93 added a comment.
This revision is now accepted and ready to land.

LGTM, Can you fix the formatting as well.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp:50
+  Vector>> Y;
+  for (int J = 0; J < Y[3].size(); ++J) {
+if (Y[3][J][7])

Not essential, but how would this handle iterating the lowest dimension. 
```lang=c++
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95771/new/

https://reviews.llvm.org/D95771

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc created this revision.
poelmanc added reviewers: sammccall, hokein.
poelmanc added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
poelmanc requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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

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

to

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95771

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
@@ -0,0 +1,70 @@
+// RUN: %check_clang_tidy %s modernize-loop-convert %t
+
+template 
+struct Vector {
+  using iterator = T*;
+  unsigned size() const;
+  const T [](int) const;
+  T [](int);
+  T *begin();
+  T *end();
+  const T *begin() const;
+  const T *end() const;
+};
+
+template 
+void copyArg(T);
+
+class TestArrayOfVector {
+  Vector 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> 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>> 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]);
+};
+
+void testVectorOfVectorIterator() {
+  Vector> Z;
+  for (Vector::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);
+};
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -722,10 +722,11 @@
   if (isa(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(ContainerExpr))
-  ContainerExpr = E->getArg(0);
+  if ( E->getOperator() != OO_Subscript )
+ContainerExpr = E->getArg(0);
 ContainerString =
 getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
ContainerExpr->getSourceRange());


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
@@ -0,0 +1,70 @@
+// RUN: %check_clang_tidy %s modernize-loop-convert %t
+
+template 
+struct Vector {
+  using iterator = T*;
+  unsigned size() const;
+  const T [](int) const;
+  T [](int);
+  T *begin();
+  T *end();
+  const T *begin() const;
+  const T *end() const;
+};
+
+template 
+void copyArg(T);
+
+class TestArrayOfVector {
+  Vector 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> X;
+
+  void foo() const {
+for