[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

By the way, much to my surprise, this didn't start diagnosing the loop i 
expected it to start diagnosing:
https://godbolt.org/z/lsJTSS
This is expected?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE360785: [clang-tidy] modernize-loop-convert: impl const 
cast iter (authored by dhinton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61827?vs=199496=199631#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827

Files:
  clang-tidy/modernize/LoopConvertCheck.cpp
  docs/clang-tidy/checks/modernize-loop-convert.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/modernize-loop-convert-basic.cpp
  test/clang-tidy/modernize-loop-convert-extra.cpp

Index: test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- test/clang-tidy/modernize-loop-convert-extra.cpp
+++ test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = []() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- test/clang-tidy/modernize-loop-convert-basic.cpp
+++ test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -791,11 +791,6 @@
   CanonicalBeginType->getPointeeType(),
   CanonicalInitVarType->getPointeeType()))
 return false;
-} else if (!Context->hasSameType(CanonicalInitVarType,
-  

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk updated this revision to Diff 199496.
torbjoernk added a comment.

minor rewording


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = []() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -258,6 +258,8 @@
   value:   'some value'
   ...
 
+.. _clang-tidy-nolint:
+
 Suppressing Undesired Diagnostics
 =
 
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

I'll be happy to commit for you, but will give it till tomorrow just to make 
sure no one else has any additional comments.

Thanks again!




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst:262
+not been used on code with a compatibility requirements of OpenMP prior to
+version 5. It is **intentional** that this check does not make any attempt to
+not issue diagnostics on OpenMP for loops.

Could you reword this last line to remove the double negative?  Perhaps 
something like:

```
This check makes no attempt to exclude incorrect diagnostics for OpenMP loops 
prior to OpenMP 5.
```


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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk updated this revision to Diff 199483.
torbjoernk edited the summary of this revision.
torbjoernk added a comment.
Herald added a subscriber: arphaman.

Addressed Roman Lebedev's comment regarding reference to `NOLINT` in the docs.

As I don't have commit rights, I'd be grateful someone else could commit this 
patch once it's fully accepted.


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = []() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda accepted this revision.
hintonda added a comment.
This revision is now accepted and ready to land.

Awesome, thanks...

LGTM, but you may want to give @alexfh or @aaron.ballman a day to comment 
before you commit.


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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Sounds good, thank you!




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst:262
+not been used on code with a compatibility requirements of OpenMP prior to
+version 5.

I would add a cross-reference to `NOLINT` documentation,
and a mention that the check **intentionally** makes no attempt
to prevent issuing these diagnostics for OpenMP loops.


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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk updated this revision to Diff 199326.
torbjoernk edited the summary of this revision.
torbjoernk added a comment.
Herald added subscribers: jdoerfert, jfb.

Fixed the issues pointed out by Don Hinton and added note on OpenMP to the 
check's docs as suggested by Roman Lebedev.

Also fixed a small typo in a comment within the tests.

Side note: Somehow, the various failing tests I had previously where due to the 
fact that `make check-clang-tools` was picking my system-installed clang-tidy-8 
and not the one I was building... o.O


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = []() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: 

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D61827#1499368 , @lebedev.ri wrote:

> In D61827#1499347 , @hintonda wrote:
>
> >
>




>> Are you saying this patch is a regression?
> 
> Not in general, no. This is most certainly an improvement for normal C++ code.

Good, then this patch can go forward.

I agree that a note that this shouldn't be run on openmp 4.  Perhaps a separate 
patch could check for that and not do anything when compiling an openmp file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

I pulled down you patch, compiled and ran it.  Once I fixed the two problems I 
mentioned, it ran clean, e.g.:

  $ bin/llvm-lit -vv 
../../llvm-project/clang-tools-extra/test/clang-tidy/modernize-loop-convert-[be]*.cpp
  -- Testing: 2 tests, 2 threads --
  PASS: Clang Tools :: clang-tidy/modernize-loop-convert-basic.cpp (1 of 2)
  PASS: Clang Tools :: clang-tidy/modernize-loop-convert-extra.cpp (2 of 2)
  Testing Time: 0.93s
Expected Passes: 2

Also, I don't think the omp issue is a regression, so I wouldn't worry about 
trying to fix it here.




Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:131
   DeclarationMatcher InitDeclMatcher =
-  varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
-   materializeTemporaryExpr(

This change is incorrect.  The matcher actually needs all three cases.  This 
explains the strange test failures you were seeing.



Comment at: 
clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp:274
+  for (S::const_iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
+printf("s has value %d\n", (*It).X);
+  }

Instead of adding a new test case, please fix the two I mentioned earlier, and 
5 more in `modernize-loop-convert-extra.cpp`.  They just need you to add the 
appropriate CHECK line.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D61827#1499347 , @hintonda wrote:

> In D61827#1499335 , @lebedev.ri 
> wrote:
>
> > In D61827#1499333 , @hintonda 
> > wrote:
> >
> > > When I asked for a test above, I understood you to say you couldn't 
> > > provide one, but If I misunderstood, by all means, please add the test.
> >
> >
> > Please do note that i have provided a testcase (godbolt link) in my very 
> > first comment, and quoted that line when replying the previous time.
> >  (Granted, that loop is not in a correct form for openmp, but the point 
> > being, the current check does not diagnose it either)
>
>
> I'm really not sure I understand what you are trying to say.




> Are you saying this patch is a regression?

Not in general, no. This is most certainly an improvement for normal C++ code.

> By "false positive", do you mean that diagnoses something it shouldn't change 
> and creates an erroneous fixit?

For normal C++ - not that i know of, can't comment.

For loops that are OpenMP loops - yes, this will be a false-positive with 
erroneous, compilation-breaking, fixit.
(To be noted, the existing fix-its in this check already are that way.)

But as i said, this is only "for your information", only to be added to docs 
(if it isn't there already).
As i have said, this can not be reliably avoided, and i don't believe a partial 
avoidance will be good.

> Of that it ignores something it should fix?

That is called false-negative. No, this isn't the case here.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D61827#1499335 , @lebedev.ri wrote:

> In D61827#1499333 , @hintonda wrote:
>
> > When I asked for a test above, I understood you to say you couldn't provide 
> > one, but If I misunderstood, by all means, please add the test.
>
>
> Please do note that i have provided a testcase (godbolt link) in my very 
> first comment, and quoted that line when replying the previous time.
>  (Granted, that loop is not in a correct form for openmp, but the point 
> being, the current check does not diagnose it either)


I'm really not sure I understand what you are trying to say.  Are you saying 
this patch is a regression?

By "false positive", do you mean that diagnoses something it shouldn't change 
and creates an erroneous fixit?  Of that it ignores something it should fix?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D61827#1499333 , @hintonda wrote:

> In D61827#1499309 , @lebedev.ri 
> wrote:
>
> > In D61827#1499306 , @hintonda 
> > wrote:
> >
> > > In D61827#1499303 , @torbjoernk 
> > > wrote:
> > >
> > > > In D61827#1499184 , 
> > > > @lebedev.ri wrote:
> > > >
> > > > > In D61827#1499183 , 
> > > > > @hintonda wrote:
> > > > >
> > > > > > In D61827#1499160 , 
> > > > > > @lebedev.ri wrote:
> > > > > >
> > > > > > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > > > > > >  Just want to point out that this will then have 
> > > > > > > "false-positives" when that loop
> > > > > > >  is an OpenMP for loop, since range-for loop is not available 
> > > > > > > until OpenMP 5.
> > > > > > >
> > > > > > > I don't think this false-positive can be avoided though, if 
> > > > > > > building without
> > > > > > >  `-fopenmp` there won't be anything about OpenMP in AST,
> > > > > > >  and thus no way to detect this case..
> > > > > >
> > > > > >
> > > > > > Could you suggest a simple test case that could be added to the 
> > > > > > test?  That way, instead of just removing the `if else` block, 
> > > > > > @torbjoernk could try to handle it.  Or perhaps exclude it from the 
> > > > > > match altogether.
> > > > >
> > > > >
> > > > > As i said, i don't see how this can be avoided in general.
> > > >
> > > >
> > > > I have to admit that I have very little experience with OpenMP and 
> > > > haven't thought of this at all. Thank you very much for bringing this 
> > > > up.
> > > >
> > > > Would it help to extend the exclusion AST matcher for iterator-based 
> > > > loops by an exclusion for loops with an ancestor of 
> > > > `ompExecutableDirective`?:
> > > >
> > > >   return forStmt(
> > > >unless(anyOf(isInTemplateInstantiation(),
> > > > hasAncestor(ompExecutableDirective(,
> > > >
> > >
> > >
> > > As a general rule, don't add anything that doesn't include a test.
> > >
> > > Since this "false positive" is apparently untestable,
> >
> >
> > How so?
>
>
> When I asked for a test above, I understood you to say you couldn't provide 
> one, but If I misunderstood, by all means, please add the test.


Please do note that i have provided a testcase (godbolt link) in my very first 
comment, and quoted that line when replying the previous time.
(Granted, that loop is not in a correct form for openmp, but the point being, 
the current check does not diagnose it either)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D61827#1499309 , @lebedev.ri wrote:

> In D61827#1499306 , @hintonda wrote:
>
> > In D61827#1499303 , @torbjoernk 
> > wrote:
> >
> > > In D61827#1499184 , @lebedev.ri 
> > > wrote:
> > >
> > > > In D61827#1499183 , @hintonda 
> > > > wrote:
> > > >
> > > > > In D61827#1499160 , 
> > > > > @lebedev.ri wrote:
> > > > >
> > > > > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > > > > >  Just want to point out that this will then have "false-positives" 
> > > > > > when that loop
> > > > > >  is an OpenMP for loop, since range-for loop is not available until 
> > > > > > OpenMP 5.
> > > > > >
> > > > > > I don't think this false-positive can be avoided though, if 
> > > > > > building without
> > > > > >  `-fopenmp` there won't be anything about OpenMP in AST,
> > > > > >  and thus no way to detect this case..
> > > > >
> > > > >
> > > > > Could you suggest a simple test case that could be added to the test? 
> > > > >  That way, instead of just removing the `if else` block, @torbjoernk 
> > > > > could try to handle it.  Or perhaps exclude it from the match 
> > > > > altogether.
> > > >
> > > >
> > > > As i said, i don't see how this can be avoided in general.
> > >
> > >
> > > I have to admit that I have very little experience with OpenMP and 
> > > haven't thought of this at all. Thank you very much for bringing this up.
> > >
> > > Would it help to extend the exclusion AST matcher for iterator-based 
> > > loops by an exclusion for loops with an ancestor of 
> > > `ompExecutableDirective`?:
> > >
> > >   return forStmt(
> > >unless(anyOf(isInTemplateInstantiation(),
> > > hasAncestor(ompExecutableDirective(,
> > >
> >
> >
> > As a general rule, don't add anything that doesn't include a test.
> >
> > Since this "false positive" is apparently untestable,
>
>
> How so?


When I asked for a test above, I understood you to say you couldn't provide 
one, but If I misunderstood, by all means, please add the test.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D61827#1499306 , @hintonda wrote:

> In D61827#1499303 , @torbjoernk 
> wrote:
>
> > In D61827#1499184 , @lebedev.ri 
> > wrote:
> >
> > > In D61827#1499183 , @hintonda 
> > > wrote:
> > >
> > > > In D61827#1499160 , 
> > > > @lebedev.ri wrote:
> > > >
> > > > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > > > >  Just want to point out that this will then have "false-positives" 
> > > > > when that loop
> > > > >  is an OpenMP for loop, since range-for loop is not available until 
> > > > > OpenMP 5.
> > > > >
> > > > > I don't think this false-positive can be avoided though, if building 
> > > > > without
> > > > >  `-fopenmp` there won't be anything about OpenMP in AST,
> > > > >  and thus no way to detect this case..
> > > >
> > > >
> > > > Could you suggest a simple test case that could be added to the test?  
> > > > That way, instead of just removing the `if else` block, @torbjoernk 
> > > > could try to handle it.  Or perhaps exclude it from the match 
> > > > altogether.
> > >
> > >
> > > As i said, i don't see how this can be avoided in general.
> >
> >
> > I have to admit that I have very little experience with OpenMP and haven't 
> > thought of this at all. Thank you very much for bringing this up.
> >
> > Would it help to extend the exclusion AST matcher for iterator-based loops 
> > by an exclusion for loops with an ancestor of `ompExecutableDirective`?:
> >
> >   return forStmt(
> >unless(anyOf(isInTemplateInstantiation(),
> > hasAncestor(ompExecutableDirective(,
> >
>
>
> As a general rule, don't add anything that doesn't include a test.
>
> Since this "false positive" is apparently untestable,


How so?

In D61827#1499306 , @hintonda wrote:

> In D61827#1499303 , @torbjoernk 
> wrote:
>
> > In D61827#1499184 , @lebedev.ri 
> > wrote:
> >
> > > In D61827#1499183 , @hintonda 
> > > wrote:
> > >
> > > > In D61827#1499160 , 
> > > > @lebedev.ri wrote:
> > > >
> > > > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > > >
> > >
> >
>




> as far as I'm concerned, it doesn't exist.  Most tests of this sort are 
> mocked up to emulate the problem so you can verify it exists and the fix 
> actually addresses it.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D61827#1499303 , @torbjoernk wrote:

> In D61827#1499184 , @lebedev.ri 
> wrote:
>
> > In D61827#1499183 , @hintonda 
> > wrote:
> >
> > > In D61827#1499160 , @lebedev.ri 
> > > wrote:
> > >
> > > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > > >  Just want to point out that this will then have "false-positives" when 
> > > > that loop
> > > >  is an OpenMP for loop, since range-for loop is not available until 
> > > > OpenMP 5.
> > > >
> > > > I don't think this false-positive can be avoided though, if building 
> > > > without
> > > >  `-fopenmp` there won't be anything about OpenMP in AST,
> > > >  and thus no way to detect this case..
> > >
> > >
> > > Could you suggest a simple test case that could be added to the test?  
> > > That way, instead of just removing the `if else` block, @torbjoernk could 
> > > try to handle it.  Or perhaps exclude it from the match altogether.
> >
> >
> > As i said, i don't see how this can be avoided in general.
>
>
> I have to admit that I have very little experience with OpenMP and haven't 
> thought of this at all. Thank you very much for bringing this up.
>
> Would it help to extend the exclusion AST matcher for iterator-based loops by 
> an exclusion for loops with an ancestor of `ompExecutableDirective`?:
>
>   return forStmt(
>unless(anyOf(isInTemplateInstantiation(),
> hasAncestor(ompExecutableDirective(,
>


As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable, as far as I'm concerned, 
it doesn't exist.  Most tests of this sort are mocked up to emulate the problem 
so you can verify it exists and the fix actually addresses it.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D61827#1499303 , @torbjoernk wrote:

> In D61827#1499184 , @lebedev.ri 
> wrote:
>
> > In D61827#1499183 , @hintonda 
> > wrote:
> >
> > > In D61827#1499160 , @lebedev.ri 
> > > wrote:
> > >
> > > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > > >  Just want to point out that this will then have "false-positives" when 
> > > > that loop
> > > >  is an OpenMP for loop, since range-for loop is not available until 
> > > > OpenMP 5.
> > > >
> > > > I don't think this false-positive can be avoided though, if building 
> > > > without
> > > >  `-fopenmp` there won't be anything about OpenMP in AST,
> > > >  and thus no way to detect this case..
> > >
> > >
> > > Could you suggest a simple test case that could be added to the test?  
> > > That way, instead of just removing the `if else` block, @torbjoernk could 
> > > try to handle it.  Or perhaps exclude it from the match altogether.
> >
> >
> > As i said, i don't see how this can be avoided in general.
>
>
> I have to admit that I have very little experience with OpenMP and haven't 
> thought of this at all. Thank you very much for bringing this up.
>
> Would it help to extend the exclusion AST matcher for iterator-based loops by 
> an exclusion for loops with an ancestor of `ompExecutableDirective`?:
>
>   return forStmt(
>unless(anyOf(isInTemplateInstantiation(),
> hasAncestor(ompExecutableDirective(,
>


Yes and no.
This **will** prevent the false-positive, but **only if** the OpenMP is 
**enabled** (`-fopenmp`).
If OpenMP is **not enabled**, then that **won't work**, because there won't be 
anything about OpenMP in AST.
I semi-strongly believe it will be less confusing to only document this 
false-positive (in check's docs),
instead of trying to prevent it, and reliably failing in half of the cases.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk added a comment.

In D61827#1499184 , @lebedev.ri wrote:

> In D61827#1499183 , @hintonda wrote:
>
> > In D61827#1499160 , @lebedev.ri 
> > wrote:
> >
> > > This will now trigger on https://godbolt.org/z/9oFMcB right?
> > >  Just want to point out that this will then have "false-positives" when 
> > > that loop
> > >  is an OpenMP for loop, since range-for loop is not available until 
> > > OpenMP 5.
> > >
> > > I don't think this false-positive can be avoided though, if building 
> > > without
> > >  `-fopenmp` there won't be anything about OpenMP in AST,
> > >  and thus no way to detect this case..
> >
> >
> > Could you suggest a simple test case that could be added to the test?  That 
> > way, instead of just removing the `if else` block, @torbjoernk could try to 
> > handle it.  Or perhaps exclude it from the match altogether.
>
>
> As i said, i don't see how this can be avoided in general.


I have to admit that I have very little experience with OpenMP and haven't 
thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by 
an exclusion for loops with an ancestor of `ompExecutableDirective`?:

  return forStmt(
   unless(anyOf(isInTemplateInstantiation(),
hasAncestor(ompExecutableDirective(,


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp:273
 
+  for (S::const_iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
+printf("s has value %d\n", (*It).X);

hintonda wrote:
> I think you need to fix the comment and add checks to these also, which is 
> what the `if else` was dealing with:
> 
>434// V.begin() returns a user-defined type 'iterator' which, since 
> it's
>435// different from const_iterator, disqualifies these loops from
>436// transformation.
>437dependent V;
>438for (dependent::const_iterator It = V.begin(), E = V.end();
>439 It != E; ++It) {
>440  printf("Fibonacci number is %d\n", *It);
>441}
>442
>443for (dependent::const_iterator It(V.begin()), E = V.end();
>444 It != E; ++It) {
>445  printf("Fibonacci number is %d\n", *It);
>446}
>447  }
> 
This seems weird and I reckon my setup is broken as also a bunch of other lit 
tests (make check-clang-tools) fail.

modernize-loop-convert-basic fails once I change this to the following:

```
  dependent V;
  for (dependent::const_iterator It = V.begin(), E = V.end();
   It != E; ++It) {
printf("Fibonacci number is %d\n", *It);
  }
  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
  // CHECK-FIXES: for (int It : V)
  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);

  for (dependent::const_iterator It(V.begin()), E = V.end();
   It != E; ++It) {
printf("Fibonacci number is %d\n", *It);
  }
  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
  // CHECK-FIXES: for (int It : V)
  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
```

However, copy outsite of llvm-lit in a minimal example, it is detected 
and fixed as expected.

Is it possible that llvm-lit picks up my (older) globally available clang-tidy?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D61827#1499160 , @lebedev.ri wrote:

> This will now trigger on https://godbolt.org/z/9oFMcB right?
>  Just want to point out that this will then have "false-positives" when that 
> loop
>  is an OpenMP for loop, since range-for loop is not available until OpenMP 5.
>
> I don't think this false-positive can be avoided though, if building without
>  `-fopenmp` there won't be anything about OpenMP in AST,
>  and thus no way to detect this case..


Could you suggest a simple test case that could be added to the test?  That 
way, instead of just removing the `if else` block, @torbjoernk could try to 
handle it.  Or perhaps exclude it from the match altogether.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D61827#1499183 , @hintonda wrote:

> In D61827#1499160 , @lebedev.ri 
> wrote:
>
> > This will now trigger on https://godbolt.org/z/9oFMcB right?
> >  Just want to point out that this will then have "false-positives" when 
> > that loop
> >  is an OpenMP for loop, since range-for loop is not available until OpenMP 
> > 5.
> >
> > I don't think this false-positive can be avoided though, if building without
> >  `-fopenmp` there won't be anything about OpenMP in AST,
> >  and thus no way to detect this case..
>
>
> Could you suggest a simple test case that could be added to the test?  That 
> way, instead of just removing the `if else` block, @torbjoernk could try to 
> handle it.  Or perhaps exclude it from the match altogether.


As i said, i don't see how this can be avoided in general.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
`-fopenmp` there won't be anything about OpenMP in AST,
and thus no way to detect this case..


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp:273
 
+  for (S::const_iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
+printf("s has value %d\n", (*It).X);

I think you need to fix the comment and add checks to these also, which is what 
the `if else` was dealing with:

   434// V.begin() returns a user-defined type 'iterator' which, since it's
   435// different from const_iterator, disqualifies these loops from
   436// transformation.
   437dependent V;
   438for (dependent::const_iterator It = V.begin(), E = V.end();
   439 It != E; ++It) {
   440  printf("Fibonacci number is %d\n", *It);
   441}
   442
   443for (dependent::const_iterator It(V.begin()), E = V.end();
   444 It != E; ++It) {
   445  printf("Fibonacci number is %d\n", *It);
   446}
   447  }



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61827



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk created this revision.
torbjoernk added reviewers: dergachev.a, hintonda, alexfh.
torbjoernk added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

modernize-loop-convert was not detecting implicit casts to
const_iterator as convertible to range-based loops:

  std::vector vec{1,2,3,4}
  for(std::vector::const_iterator i = vec.begin();
  i != vec.end();
  ++i) { }

Thanks to Don Hinton for advice on cfe-dev.

Also simplify AST matcher for matching iterator-based loops.

Thanks to Artem Dergachev for spotting it on cfe-dev.

Fixes PR#35082


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D61827

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


Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -270,6 +270,13 @@
   // CHECK-FIXES: for (auto & P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
+  for (S::const_iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
+printf("s has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto It : Ss)
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
+
   for (S::const_iterator It = Ss.cbegin(), E = Ss.cend(); It != E; ++It) {
 printf("s has value %d\n", (*It).X);
   }
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
@@ -128,10 +128,7 @@
   .bind(BeginCallName);
 
   DeclarationMatcher InitDeclMatcher =
-  varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
-   materializeTemporaryExpr(
-   
ignoringParenImpCasts(BeginCallMatcher)),
-   hasDescendant(BeginCallMatcher
+  varDecl(hasInitializer(hasDescendant(BeginCallMatcher)))
   .bind(InitVarName);
 
   DeclarationMatcher EndDeclMatcher =
@@ -791,11 +788,6 @@
   CanonicalBeginType->getPointeeType(),
   CanonicalInitVarType->getPointeeType()))
 return false;
-} else if (!Context->hasSameType(CanonicalInitVarType,
- CanonicalBeginType)) {
-  // Check for qualified types to avoid conversions from non-const to const
-  // iterator types.
-  return false;
 }
   } else if (FixerKind == LFK_PseudoArray) {
 // This call is required to obtain the container.


Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -270,6 +270,13 @@
   // CHECK-FIXES: for (auto & P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
+  for (S::const_iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
+printf("s has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto It : Ss)
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", It.X);
+
   for (S::const_iterator It = Ss.cbegin(), E = Ss.cend(); It != E; ++It) {
 printf("s has value %d\n", (*It).X);
   }
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
@@ -128,10 +128,7 @@
   .bind(BeginCallName);
 
   DeclarationMatcher InitDeclMatcher =
-  varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
-   materializeTemporaryExpr(
-   ignoringParenImpCasts(BeginCallMatcher)),
-   hasDescendant(BeginCallMatcher
+  varDecl(hasInitializer(hasDescendant(BeginCallMatcher)))
   .bind(InitVarName);
 
   DeclarationMatcher EndDeclMatcher =
@@ -791,11 +788,6 @@
   CanonicalBeginType->getPointeeType(),
   CanonicalInitVarType->getPointeeType()))
 return false;
-} else if (!Context->hasSameType(CanonicalInitVarType,
- CanonicalBeginType)) {
-  // Check for qualified types to avoid conversions from non-const to const
-  // iterator types.
-  return false;
 }
   } else