[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 121740.
barancsuk marked 2 inline comments as done.
barancsuk added a comment.

Address review comments: fix missing full stops at the end of comments


https://reviews.llvm.org/D38688

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tidy/misc/RedundantExpressionCheck.h
  docs/clang-tidy/checks/misc-redundant-expression.rst
  test/clang-tidy/misc-redundant-expression.cpp

Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -15,69 +15,71 @@
 extern int bar(int x);
 extern int bat(int x, int y);
 
-int Test(int X, int Y) {
+int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X || X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X != (((X return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X + 1 == X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 != X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 <= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 >= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
 
   if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
   if (P.a[X - P.x] != P.a[X - P.x]) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
 
   if ((int)X < (int)X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
+  if (int(X) < int(X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
   if ( + "dummy" == + 

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:41
+static const llvm::StringSet<> KnownBannedMacroNames = {"EAGAIN", 
"EWOULDBLOCK",
+  "SIGCLD", "SIGCHLD"};
 

xazax.hun wrote:
> Is this block clang formatted? The indentation of the second line looks 
> strange. 
It was, but the second line shifted somehow. I fixed it in the updated diff.


https://reviews.llvm.org/D38688



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

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 121323.
barancsuk added a comment.

Fix shifted line


https://reviews.llvm.org/D38688

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tidy/misc/RedundantExpressionCheck.h
  docs/clang-tidy/checks/misc-redundant-expression.rst
  test/clang-tidy/misc-redundant-expression.cpp

Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -15,69 +15,71 @@
 extern int bar(int x);
 extern int bat(int x, int y);
 
-int Test(int X, int Y) {
+int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X || X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X != (((X return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X + 1 == X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 != X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 <= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 >= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
 
   if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
   if (P.a[X - P.x] != P.a[X - P.x]) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
 
   if ((int)X < (int)X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
+  if (int(X) < int(X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
   if ( + "dummy" == + "dummy") return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are 

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:633-635
+// The function discards  (X < M1 && X <= M2), because it can always be
+// simplified to X < M, where M is the smaller one of the two macro
+// values.

aaron.ballman wrote:
> This worries me slightly for macros because those values can be overridden 
> for different compilation targets. So the diagnostic may be correct for a 
> particular configuration of the macros, but incorrect for another. Have you 
> tested this against any large code bases to see what the quality of the 
> diagnostics looks like?
Thank you for your insight! 

Now that I come to think of it, although these expressions are redundant for 
every configuration, there is no adequate simplification for each compilation 
target, because they might change their behavior if the values vary.

A good example is the following expression, that is always redundant regardless 
of the values that the macros hold.
However, the particular macro that causes the redundancy can vary from one 
compilation target to another.
```
(X < MACRO1 || X < MACRO2)
```
If MACRO1 > MACRO2, the appropriate simplification is `(X < MACRO1 )`, 
and  if MACRO1 < MACRO2, it is `(X < MACRO2)`.

Even expressions, like `(X < MACRO1 || X != MACRO2)`, that can almost always be 
simplified to `(X != MACRO2)`, change their behavior, if MACRO1 is equal to 
MACRO2  (then they are always true), and can be considered conscious 
development choices.

In conclusion, I think, it might be better now to skip these expressions, and 
check for only the ones that contain the same macro twice, like `X < MACRO && X 
> MACRO`.



https://reviews.llvm.org/D38688



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

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 120820.
barancsuk marked 17 inline comments as done.
barancsuk added a comment.

Address review comments, discard ambiguously redundant macro expressions


https://reviews.llvm.org/D38688

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tidy/misc/RedundantExpressionCheck.h
  docs/clang-tidy/checks/misc-redundant-expression.rst
  test/clang-tidy/misc-redundant-expression.cpp

Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -15,69 +15,71 @@
 extern int bar(int x);
 extern int bat(int x, int y);
 
-int Test(int X, int Y) {
+int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X || X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X != (((X return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X + 1 == X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 != X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 <= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 >= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
 
   if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
   if (P.a[X - P.x] != P.a[X - P.x]) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
 
   if ((int)X < (int)X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
+  if (int(X) < int(X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
   if ( + "dummy" 

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:510
+// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5
+static void retrieveConstExprFromBothSides(const BinaryOperator *,
+   BinaryOperatorKind ,

xazax.hun wrote:
> I would prefer to return a pair of pointer to be returned to output 
> parameters. 
I think, since 'Retrieve'  functions have multiple output parameters, they 
should remain unchanged. 

'Retrieve' methods are responsible for splitting expressions into parts (e.g.: 
into opcode and constant integer value) and storing each part into the 
corresponding output parameter. Therefore it is more reasonable to return 
whether the operation was successful than returning one of these parameters.



Comment at: test/clang-tidy/misc-redundant-expression.cpp:387
   // Should not match.
-  if (X == 10 && Y == 10) return 1;
-  if (X != 10 && X != 12) return 1;

xazax.hun wrote:
> Why did you remove these test cases?
The above test cases were not removed, just moved into the function 
`TestLogical`, as they contain only equality operators, and no relational 
operators.


https://reviews.llvm.org/D38688



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

[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 119452.
barancsuk marked 8 inline comments as done.
barancsuk added a comment.
Herald added a subscriber: whisperity.

Address review comments.


https://reviews.llvm.org/D38688

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tidy/misc/RedundantExpressionCheck.h
  docs/clang-tidy/checks/misc-redundant-expression.rst
  test/clang-tidy/misc-redundant-expression.cpp

Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -15,69 +15,71 @@
 extern int bar(int x);
 extern int bat(int x, int y);
 
-int Test(int X, int Y) {
+int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X || X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X != (((X return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X + 1 == X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 != X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 <= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 >= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
 
   if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
   if (P.a[X - P.x] != P.a[X - P.x]) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
 
   if ((int)X < (int)X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
+  if (int(X) < int(X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
   if ( + "dummy" == + 

[PATCH] D38688: Misc redundant expressions checker updated for macros

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk created this revision.
barancsuk added a project: clang-tools-extra.

Redundant Expression Checker is updated to be able to detect expressions that 
contain macro constants. Also, other small details are modified to improve the 
current implementation.

The improvements in detail are as follows:

**1.)** Binary and ternary operator expressions containing two constants, with 
at least one of them from a macro, are detected and tested for redundancy.

Macro expressions are treated somewhat differently from other expressions, 
because the particular values of macros can vary across builds. They can be 
considered correct and intentional, even if macro values equal, produce ranges 
that exclude each other or fully overlap, etc. The correctness of a macro 
expression is decided based on solely the operators involved in the expression.

Examples:

The following expression is always redundant, independently of the macro 
values, because the subexpression containing the larger constant can be 
eliminated without changing the meaning of the expression:

  (X < MACRO && X < OTHER_MACRO)

On the contrary, the following expression is considered meaningful, because 
some macro values (e.g.:  MACRO = 3, OTHER_MACRO = 2) produce a valid interval:

  (X < MACRO && X > OTHER_MACRO)

**2.) **The code structure is slightly modified: typos are corrected, comments 
are added and some functions are renamed to improve comprehensibility, both in 
the checker and the test file. A few test cases are moved to another function.

**3.)** The checker is now able to detect redundant CXXFunctionalCastExprs as 
well (it matched only CStyleCastExprs before). A corresponding test case is 
added.


https://reviews.llvm.org/D38688

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tidy/misc/RedundantExpressionCheck.h
  docs/clang-tidy/checks/misc-redundant-expression.rst
  test/clang-tidy/misc-redundant-expression.cpp

Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -15,69 +15,71 @@
 extern int bar(int x);
 extern int bat(int x, int y);
 
-int Test(int X, int Y) {
+int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X || X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X != (((X return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X + 1 == X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are 

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added a comment.

@alexfh, have you had a chance to look at the results yet?


https://reviews.llvm.org/D20689



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

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added a comment.

I attached the results of the tests.
The warnings are categorized into false positives and renaming opportunities.

F5376033: PostgreSQL 

F5376032: FFmpeg 

F5376031: LLVM 

F5376030: OpenSSL 

F5376029: Xerces 

F5376028: Cpython 


https://reviews.llvm.org/D20689



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

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added a comment.

@alexfh, would you mind taking a look at the changes that have been introduced 
in the new patch?

The main improvements are:

- The checker has been shifted to the module `readability`.
- It is checked, whether implicit type conversion is possible from the argument 
to the parameter.

I have run the modified checker on some large code bases with the following 
results:
 (The errors were categorized subjectively.)

- **PostgreSQL:** 32 renaming opportunities of 39 warnings
- **Cpython:** 10 renaming opportunities of 15 warnings
- **Xerces:** 6 renaming opportunities of 8 warnings
- **FFmpeg**:  5 renaming opportunities of 9 warnings
- **OpenSSL:** 3 renaming opportunities of 4 warnings
- **LLVM:** 20 renaming opportunities of 44 warnings

This article provides some evidence to support the feasibility of the checker 
as well. 
F5358417: icse2016-names.pdf  
The authors have proven that argument names are generally very similar to the 
corresponding parameters' names. 
The presented empirical evidence also shows, that argument and parameter name 
dissimilarities are strong indicators of incorrect argument usages, or they 
identify renaming opportunities to improve code readability. 
Moreover the authors have even found 3 existing bugs in open source projects.


https://reviews.llvm.org/D20689



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

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 113830.
barancsuk added a comment.

Check if argument and parameter numbers differ, add test cases for functions 
with default parameters


https://reviews.llvm.org/D20689

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang-tidy/readability/SuspiciousCallArgumentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-redundant-expression.rst
  docs/clang-tidy/checks/readability-suspicious-call-argument.rst
  test/clang-tidy/misc-redundant-expression.cpp
  test/clang-tidy/readability-suspicious-call-argument.cpp

Index: test/clang-tidy/readability-suspicious-call-argument.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-suspicious-call-argument.cpp
@@ -0,0 +1,391 @@
+// RUN: %check_clang_tidy %s readability-suspicious-call-argument %t -- -- -std=c++11
+
+void foo_1(int aa, int bb) {}
+
+void foo_2(int source, int aa) {}
+
+void foo_3(int valToRet, int aa) {}
+
+void foo_4(int pointer, int aa) {}
+
+void foo_5(int aa, int bb, int cc, ...) {}
+
+void foo_6(const int dd, bool ) {}
+
+void foo_7(int aa, int bb, int cc, int ff = 7) {}
+
+// Test functions for convertible argument--parameter types.
+void fun(const int );
+void fun2() {
+  int m = 3;
+  fun(m);
+}
+
+// Test cases for parameters of const reference and value.
+void value_const_reference(int ll, const int );
+
+void const_ref_value_swapped() {
+  const int  = 42;
+  const int  = 42;
+  value_const_reference(kk, ll);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: kk (ll) is swapped with ll (kk). [readability-suspicious-call-argument]
+}
+
+// Const, non const references.
+void const_nonconst_parameters(const int , int );
+
+void const_nonconst_swap1() {
+  const int& nn = 42;
+  int mm;
+  // Do not check, because non-const reference parameter cannot bind to const reference argument.
+  const_nonconst_parameters(nn, mm);
+}
+
+void const_nonconst_swap3() {
+  const int nn = 42;
+  int m = 42;
+  int  = m;
+  // Do not check, const int does not bind to non const reference.
+  const_nonconst_parameters(nn, mm);
+}
+
+void const_nonconst_swap2() {
+  int nn;
+  int mm;
+  // Check for swapped arguments. (Both arguments are non-const.)
+  const_nonconst_parameters(
+  nn,
+  mm);
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: nn (mm) is swapped with mm (nn). [readability-suspicious-call-argument]
+}
+
+void const_nonconst_pointers(const int* mm, int* nn);
+void const_nonconst_pointers2(const int* mm, const int* nn);
+
+void const_nonconst_pointers_swapped(){
+int* mm;
+const int* nn;
+const_nonconst_pointers(nn, mm);
+}
+
+void const_nonconst_pointers_swapped2() {
+  const int *mm;
+  int *nn;
+  const_nonconst_pointers2(nn, mm);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: nn (mm) is swapped with mm (nn). [readability-suspicious-call-argument]
+}
+
+// Test cases for pointers and arrays.
+void pointer_array_parameters(
+int *pp, int qq[4]);
+
+void pointer_array_swap() {
+  int qq[5];
+  int *pp;
+  // Check for swapped arguments. An array implicitly converts to a pointer.
+  pointer_array_parameters(qq, pp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: qq (pp) is swapped with pp (qq). [readability-suspicious-call-argument]
+}
+
+// Test cases for multilevel pointers.
+void multilevel_pointer_parameters(int *const **pp,
+   const int *const *volatile const *qq);
+void multilevel_pointer_parameters2(
+char *nn, char *volatile *const *const *const *const );
+
+typedef float T;
+typedef T * S;
+typedef S *const volatile R;
+typedef R * Q;
+typedef Q * P;
+typedef P * O;
+void multilevel_pointer_parameters3(float **const volatile ***rr, O );
+
+void multilevel_pointer_swap() {
+  int *const ** qq;
+  int *const ** pp;
+  multilevel_pointer_parameters(qq, pp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: qq (pp) is swapped with pp (qq). [readability-suspicious-call-argument]
+
+  char *mm;
+  char *nn;
+  multilevel_pointer_parameters2(mm, nn);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: mm (nn) is swapped with nn (mm). [readability-suspicious-call-argument]
+
+  float **const volatile *** rr;
+  float **const volatile *** ss;
+  multilevel_pointer_parameters3(ss, rr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ss (rr) is swapped with rr (ss). [readability-suspicious-call-argument]
+}
+
+void multilevel_pointer_parameters4(char pp,
+ 

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 113376.
barancsuk added a comment.

Major changes that have been made since the last update are as follows:

- It is checked, whether implicit type conversion is possible from the argument 
to the other parameter. The following conversion rules are considered:
  - Arithmetic type conversions
  - Pointer to pointer conversion (for multilevel pointers and also 
base/derived class pointers)
  - Array to pointer conversion
  - Function to function-pointer conversion
  - CV-qualifier compatibility is checked as well.

- The calculation of a heuristic`s result and the comparison of this result 
with the corresponding threshold value are performed by the same method, the 
heuristic`s function.
- The heuristic`s function is called only if the heuristic itself is turned on 
by the configuration settings.

**Remark**:
Implicit conversion rules of C  are not checked separately, because, as I 
experienced when testing the checker on a larger code base, deeming all 
pointers convertible results in several false positives.


https://reviews.llvm.org/D20689

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang-tidy/readability/SuspiciousCallArgumentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-suspicious-call-argument.rst
  test/clang-tidy/readability-suspicious-call-argument.cpp

Index: test/clang-tidy/readability-suspicious-call-argument.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-suspicious-call-argument.cpp
@@ -0,0 +1,379 @@
+// RUN: %check_clang_tidy %s readability-suspicious-call-argument %t -- -- -std=c++11
+
+void foo_1(int aa, int bb) {}
+
+void foo_2(int source, int aa) {}
+
+void foo_3(int valToRet, int aa) {}
+
+void foo_4(int pointer, int aa) {}
+
+void foo_5(int aa, int bb, int cc, ...) {}
+
+void foo_6(const int dd, bool ) {}
+
+// Test functions for convertible argument--parameter types.
+void fun(const int );
+void fun2() {
+  int m = 3;
+  fun(m);
+}
+
+// Test cases for parameters of const reference and value.
+void value_const_reference(int ll, const int );
+
+void const_ref_value_swapped() {
+  const int  = 42;
+  const int  = 42;
+  value_const_reference(kk, ll);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: kk (ll) is swapped with ll (kk). [readability-suspicious-call-argument]
+}
+
+// Const, non const references.
+void const_nonconst_parameters(const int , int );
+
+void const_nonconst_swap1() {
+  const int& nn = 42;
+  int mm;
+  // Do not check, because non-const reference parameter cannot bind to const reference argument.
+  const_nonconst_parameters(nn, mm);
+}
+
+void const_nonconst_swap3() {
+  const int nn = 42;
+  int m = 42;
+  int  = m;
+  // Do not check, const int does not bind to non const reference.
+  const_nonconst_parameters(nn, mm);
+}
+
+void const_nonconst_swap2() {
+  int nn;
+  int mm;
+  // Check for swapped arguments. (Both arguments are non-const.)
+  const_nonconst_parameters(
+  nn,
+  mm);
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: nn (mm) is swapped with mm (nn). [readability-suspicious-call-argument]
+}
+
+void const_nonconst_pointers(const int* mm, int* nn);
+void const_nonconst_pointers2(const int* mm, const int* nn);
+
+void const_nonconst_pointers_swapped(){
+int* mm;
+const int* nn;
+const_nonconst_pointers(nn, mm);
+}
+
+void const_nonconst_pointers_swapped2() {
+  const int *mm;
+  int *nn;
+  const_nonconst_pointers2(nn, mm);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: nn (mm) is swapped with mm (nn). [readability-suspicious-call-argument]
+}
+
+// Test cases for pointers and arrays.
+void pointer_array_parameters(
+int *pp, int qq[4]);
+
+void pointer_array_swap() {
+  int qq[5];
+  int *pp;
+  // Check for swapped arguments. An array implicitly converts to a pointer.
+  pointer_array_parameters(qq, pp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: qq (pp) is swapped with pp (qq). [readability-suspicious-call-argument]
+}
+
+// Test cases for multilevel pointers.
+void multilevel_pointer_parameters(int *const **pp,
+   const int *const *volatile const *qq);
+void multilevel_pointer_parameters2(
+char *nn, char *volatile *const *const *const *const );
+
+typedef float T;
+typedef T * S;
+typedef S *const volatile R;
+typedef R * Q;
+typedef Q * P;
+typedef P * O;
+void multilevel_pointer_parameters3(float **const volatile ***rr, O );
+
+void multilevel_pointer_swap() {
+  int *const ** qq;
+  int *const ** pp;
+  multilevel_pointer_parameters(qq, pp);
+  

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk commandeered this revision.
barancsuk added a reviewer: varjujan.
barancsuk added a comment.
Herald added a subscriber: baloghadamsoftware.

Since the previous author, @varjujan is not available anymore, I take over the 
role of the author of this revision.


https://reviews.llvm.org/D20689



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

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk marked 3 inline comments as done.
barancsuk added inline comments.



Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:55
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}

alexfh wrote:
> Line wrapping gone bad. I guess you can just remove the second line. Same 
> below.
I think the line wrapping and the typo have already been corrected in a later 
version.


Repository:
  rL LLVM

https://reviews.llvm.org/D35937



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

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+  memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+  varDecl(hasStaticStorageDuration(,
+ unless(isInTemplateInstantiation()))

aaron.ballman wrote:
> barancsuk wrote:
> > aaron.ballman wrote:
> > > Why not use `isStaticStorageClass()` here as well?
> > Unfortunately, `isStaticStorageClass()` misses variable declarations that 
> > do not contain the static keyword, including definitions of static 
> > variables outside of their class.
> > However, `hasStaticStorageDuration()` has no problem finding all static 
> > variable declarations correctly. 
> Under what circumstances would that make a difference? If the variable is 
> declared within the class with the static storage specifier, that declaration 
> should be sufficient for the check, regardless of how the definition looks, 
> no?
> 
> If it does make a difference, can you add a test case that demonstrates that?
`isStaticStorageClass()` does fail for the current test cases.

The reason for that is the function `hasDecl()`, that, for static variables 
with multiple declarations,  may return a declaration that does not contain the 
`static` keyword.
For example, it finds `int C::x = 0;` rather than `static int x;`
Then `isStaticStorageClass()` discards it.

However, `hasStaticStorageDuration()` works fine, because all declarations are 
static storage duration, regardless of whether the `static` keyword is present 
or not.


https://reviews.llvm.org/D35937



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

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 110175.
barancsuk marked 4 inline comments as done.
barancsuk added a comment.

Further reviews addressed


https://reviews.llvm.org/D35937

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
  clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
  
test/clang-tidy/readability-static-accessed-through-instance-nesting-threshold.cpp
  test/clang-tidy/readability-static-accessed-through-instance.cpp

Index: test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,222 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+  static void foo();
+  static int x;
+  int nsx;
+  void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo();   // OK, foo() is accessed inside the struct.
+  }
+  void ns() const;
+};
+
+int C::x = 0;
+
+struct CC {
+  void foo();
+  int x;
+};
+
+template  struct CT {
+  static T foo();
+  static T x;
+  int nsx;
+  void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo();   // OK, foo() is accessed inside the struct.
+  }
+};
+
+// Expressions with side effects
+C (int, int, int, int);
+void g() {
+  f(1, 2, 3, 4).x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  f(1, 2, 3, 4).x;{{$}}
+}
+
+int i(int &);
+void j(int);
+C h();
+bool a();
+int k(bool);
+
+void f(C c) {
+  j(i(h().x));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: static member
+  // CHECK-FIXES: {{^}}  j(i(h().x));{{$}}
+
+  // The execution of h() depends on the return value of a().
+  j(k(a() && h().x));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static member
+  // CHECK-FIXES: {{^}}  j(k(a() && h().x));{{$}}
+
+  if ([c]() {
+c.ns();
+return c;
+  }().x == 15)
+;
+  // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
+  // CHECK-FIXES: {{^}}  if ([c]() {{{$}}
+}
+
+// Nested specifiers
+namespace N {
+struct V {
+  static int v;
+  struct T {
+static int t;
+struct U {
+  static int u;
+};
+  };
+};
+}
+
+void f(N::V::T::U u) {
+  N::V v;
+  v.v = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  N::V::v = 12;{{$}}
+
+  N::V::T w;
+  w.t = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  N::V::T::t = 12;{{$}}
+
+  // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+  u.u = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  u.u = 12;{{$}}
+
+  using B = N::V::T::U;
+  B b;
+  b.u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  B::u;{{$}}
+}
+
+// Templates
+template  T CT::x;
+
+template  struct CCT {
+  T foo();
+  T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template  void f(T t, C c) {
+  t.x; // OK, t is a template parameter.
+  c.x; // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::x; // 1{{$}}
+}
+
+template  struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template  void h() {
+  S sN;
+  sN.x; // OK, value of N affects whether x is static or not.
+
+  S<2> s2;
+  s2.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+  C *c1 = new C();
+  c1->foo(); // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::foo(); // 1{{$}}
+  c1->x; // 2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::x; // 2{{$}}
+  c1->nsx; // OK, nsx is a non-static member.
+
+  const C *c2 = new C();
+  c2->foo(); // 2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::foo(); // 2{{$}}
+
+  C::foo(); // OK, foo() is accessed using a qualified-id.
+  C::x; // OK, x is accessed using a qualified-id.
+
+  D d;
+  d.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  D::foo();{{$}}
+  d.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  D::x;{{$}}
+
+  E e;
+  e.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  E::foo();{{$}}
+  e.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // 

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23
+  memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),
+  varDecl(hasStaticStorageDuration(,
+ unless(isInTemplateInstantiation()))

aaron.ballman wrote:
> Why not use `isStaticStorageClass()` here as well?
Unfortunately, `isStaticStorageClass()` misses variable declarations that do 
not contain the static keyword, including definitions of static variables 
outside of their class.
However, `hasStaticStorageDuration()` has no problem finding all static 
variable declarations correctly. 



Comment at: 
docs/clang-tidy/checks/readability-static-accessed-through-instance.rst:7
+Checks for member expressions that access static members through instances, and
+replaces them with the corresponding expressions that use a more readable `::` 
operator.
+

aaron.ballman wrote:
> Eugene.Zelenko wrote:
> > Is :: operator really?
> Same wording suggestion here as above.
The suggested wording is indeed a clearer one, and I rewrote the descriptions 
and the documentation accordingly, however I found, that in the C++ 
International Standard `::` is referred to as a scope (resolution) operator.



Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through 
instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
+}

aaron.ballman wrote:
> baloghadamsoftware wrote:
> > xazax.hun wrote:
> > > baloghadamsoftware wrote:
> > > > aaron.ballman wrote:
> > > > > This fix-it worries me because it changes the semantics of the code. 
> > > > > The function `f()` is no longer called, and so this isn't a valid 
> > > > > source transformation.
> > > > Maybe the correct fix would be here f(1, 2, 3, 4); C::x;
> > > Maybe for now we should just skip this cases. Expr::HasSideEffects might 
> > > be a  good candidate to filter these cases. 
> > I think including the expression with side effect before the member access 
> > (as I suggested) is not more complicated than skipping these cases.
> Please ensure you handle the more complex cases then, such as:
> ```
> struct S {
>   static int X;
> };
> 
> void g(int &);
> S h();
> 
> void f(S s) {
>   g(h().X);
>   if ([s]() { return s; }().X == 15)
> ;
> }
> ```
Expressions with side effects introduce some troublesome cases.
For example, in the following code, the static member expression, `h().x` does 
not get evaluated if a() returns false. 

```
struct C {
  static int x;
};

void j(int);
int k(bool);
bool a();
C h();

void g(){
  j(k(a() && h().x));
}
```

Maybe for now, the check should filter these expressions with side effects.
They might better be addressed in a follow-up patch.


https://reviews.llvm.org/D35937



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

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 109120.
barancsuk marked 7 inline comments as done.
barancsuk added a comment.

Address review comments


https://reviews.llvm.org/D35937

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
  clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
  test/clang-tidy/readability-static-accessed-through-instance.cpp

Index: test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,222 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+  static void foo();
+  static int x;
+  int nsx;
+  void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo();   // OK, foo() is accessed inside the struct.
+  }
+  void ns() const;
+};
+
+int C::x = 0;
+
+struct CC {
+  void foo();
+  int x;
+};
+
+template  struct CT {
+  static T foo();
+  static T x;
+  int nsx;
+  void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using a qualified-id.
+foo();   // OK, foo() is accessed inside the struct.
+  }
+};
+
+// Expressions with side effects
+C (int, int, int, int);
+void g() {
+  f(1, 2, 3, 4).x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  f(1, 2, 3, 4).x;{{$}}
+}
+
+int i(int &);
+void j(int);
+C h();
+bool a();
+int k(bool);
+
+void f(C c) {
+  j(i(h().x));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: static member
+  // CHECK-FIXES: {{^}}  j(i(h().x));{{$}}
+
+  // The execution of h() depends on the return value of a().
+  j(k(a() && h().x));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static member
+  // CHECK-FIXES: {{^}}  j(k(a() && h().x));{{$}}
+
+  if ([c]() {
+c.ns();
+return c;
+  }().x == 15)
+;
+  // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: static member
+  // CHECK-FIXES: {{^}}  if ([c]() {{{$}}
+}
+
+// Nested specifiers
+namespace N {
+struct V {
+  static int v;
+  struct T {
+static int t;
+struct U {
+  static int u;
+};
+  };
+};
+}
+
+void f(N::V::T::U u) {
+  N::V v;
+  v.v = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  N::V::v = 12;{{$}}
+
+  N::V::T w;
+  w.t = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  N::V::T::t = 12;{{$}}
+
+  // u.u is not changed to N::V::T::U::u; because the nesting level is over 3.
+  u.u = 12;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  u.u = 12;{{$}}
+
+  using B = N::V::T::U;
+  B b;
+  b.u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  B::u;{{$}}
+}
+
+// Templates
+template  T CT::x;
+
+template  struct CCT {
+  T foo();
+  T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template  void f(T t, C c) {
+  t.x; // OK, t is a template parameter.
+  c.x; // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::x; // 1{{$}}
+}
+
+template  struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template  void h() {
+  S sN;
+  sN.x; // OK, value of N affects whether x is static or not.
+
+  S<2> s2;
+  s2.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+  C *c1 = new C();
+  c1->foo(); // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::foo(); // 1{{$}}
+  c1->x; // 2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::x; // 2{{$}}
+  c1->nsx; // OK, nsx is a non-static member.
+
+  const C *c2 = new C();
+  c2->foo(); // 2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  C::foo(); // 2{{$}}
+
+  C::foo(); // OK, foo() is accessed using a qualified-id.
+  C::x; // OK, x is accessed using a qualified-id.
+
+  D d;
+  d.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  D::foo();{{$}}
+  d.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  D::x;{{$}}
+
+  E e;
+  e.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  E::foo();{{$}}
+  e.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  E::x;{{$}}
+
+  CC *cc = new CC;
+
+  f(*c1, *c1);
+  f(*cc, *c1);
+
+ 

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk created this revision.
barancsuk added a project: clang-tools-extra.
Herald added subscribers: baloghadamsoftware, xazax.hun, whisperity, 
JDevlieghere, mgorny.

Checks for member expressions that access static members through instances, and
replaces them with the corresponding expressions that use a more readable `::` 
operator.

Example:

The following code:

  struct C {
static void foo();
static int x;
  };
  
  C *c1=new C();
  c1->foo();
  c1->x;

is changed to:

  C::foo();
  C::x;


Repository:
  rL LLVM

https://reviews.llvm.org/D35937

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
  clang-tidy/readability/StaticAccessedThroughInstanceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-static-accessed-through-instance.rst
  test/clang-tidy/readability-static-accessed-through-instance.cpp

Index: test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -0,0 +1,137 @@
+// RUN: %check_clang_tidy %s readability-static-accessed-through-instance %t
+
+struct C {
+  static void foo();
+  static int x;
+  int nsx;
+  void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using the scope operator.
+foo();   // OK, foo() is accessed inside the struct.
+  }
+};
+
+struct CC {
+  void foo();
+  int x;
+};
+
+template  struct CT {
+  static T foo();
+  static T x;
+  int nsx;
+  void mf() {
+(void)// OK, x is accessed inside the struct.
+(void)::x; // OK, x is accessed using the scope operator.
+foo();   // OK, foo() is accessed inside the struct.
+  }
+};
+
+C (int, int, int, int);
+void g() {
+  f(1, 2, 3, 4).x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
+}
+
+template  T CT::x;
+
+template  struct CCT {
+  T foo();
+  T x;
+};
+
+typedef C D;
+
+using E = D;
+
+#define FOO(c) c.foo()
+#define X(c) c.x
+
+template  void f(T t, C c) {
+  t.x; // OK, t is a template parameter.
+  c.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
+}
+
+template  struct S { static int x; };
+
+template <> struct S<0> { int x; };
+
+template  void h() {
+  S sN;
+  sN.x; // OK, value of N affects whether x is static or not.
+
+  S<2> s2;
+  s2.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  S<2>::x;{{$}}
+}
+
+void static_through_instance() {
+  C *c1 = new C();
+  c1->foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::foo();{{$}}
+  c1->x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  C::x;{{$}}
+  c1->nsx; // OK, nsx is a non-static member.
+
+  C::foo(); // OK, foo() is accessed using the scope operator.
+  C::x; // OK, x is accessed using the scope operator.
+
+  D d;
+  d.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  D::foo();{{$}}
+  d.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  D::x;{{$}}
+
+  E e;
+  e.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  E::foo();{{$}}
+  e.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  E::x;{{$}}
+
+  CC *cc = new CC;
+
+  f(*c1, *c1);
+  f(*cc, *c1);
+
+  // Macros: OK, macros are not checked.
+  FOO((*c1));
+  X((*c1));
+  FOO((*cc));
+  X((*cc));
+
+  // Templates
+  CT ct;
+  ct.foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  CT::foo();{{$}}
+  ct.x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through
+  // instance  [readability-static-accessed-through-instance]
+  // CHECK-FIXES: {{^}}  CT::x;{{$}}
+  ct.nsx; // OK, nsx is a non-static member
+
+  CCT cct;
+  cct.foo(); // OK, CCT has no static members.
+ 

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: test/clang-tidy/modernize-unary-static-assert.cpp:16
+  // CHECK-FIXES: {{^}}  FOO{{$}}
+  static_assert(sizeof(a) <= 17, MSG);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when

aaron.ballman wrote:
> This probably should not diagnose, but definitely should not provide a fixit 
> -- just because the macro is empty doesn't mean it will *always* be empty. 
> Consider:
> ```
> #if FOO
> #define MSG ""
> #else
> #define MSG "Not empty"
> #endif
> ```
> I think diagnosing this case is just as likely to be a false-positive as not 
> diagnosing, so my preference is to be silent instead of chatty. However, 
> maybe there are other opinions.
You are right. Unfortunately, the message is also from a macro expansion when 
the whole static assert is from a macro expansion. Maybe it is possible to 
check whether they are from the same macro, but I think it might better be 
addressed in a separate patch if there is a need to support that case. 


https://reviews.llvm.org/D35257



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

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 106195.
barancsuk marked 4 inline comments as done.
barancsuk added a comment.

- No longer warn when the message is from a macro expansion


https://reviews.llvm.org/D35257

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-unary-static-assert.rst
  test/clang-tidy/modernize-unary-static-assert.cpp

Index: test/clang-tidy/modernize-unary-static-assert.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-unary-static-assert.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s modernize-unary-static-assert %t -- -- -std=c++1z
+
+#define FOO static_assert(sizeof(a) <= 15, "");
+#define MSG ""
+
+void f_textless(int a) {
+  static_assert(sizeof(a) <= 10, "");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when the string literal is an empty string [modernize-unary-static-assert]
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 10 );{{$}}
+  static_assert(sizeof(a) <= 12, L"");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 12 );{{$}}
+  FOO
+  // CHECK-FIXES: {{^}}  FOO{{$}}
+  static_assert(sizeof(a) <= 17, MSG);
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 17, MSG);{{$}}
+}
+
+void f_with_tex(int a) {
+  static_assert(sizeof(a) <= 10, "Size of variable a is out of range!");
+}
+
+void f_unary(int a) { static_assert(sizeof(a) <= 10); }
+
+void f_incorrect_assert() { static_assert(""); }
Index: docs/clang-tidy/checks/modernize-unary-static-assert.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-unary-static-assert.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - modernize-unary-static-assert
+
+modernize-unary-static-assert
+=
+
+The check diagnoses any ``static_assert`` declaration with an empty string literal
+and provides a fix-it to replace the declaration with a single-argument ``static_assert`` declaration.
+
+The check is only applicable for C++17 and later code.
+
+The following code:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10, "");
+  }
+
+is replaced by:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10);
+  }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -132,6 +132,7 @@
modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
+   modernize-unary-static-assert
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -108,6 +108,12 @@
   Finds and replaces explicit calls to the constructor in a return statement by
   a braced initializer list so that the return type is not needlessly repeated.
 
+- New `modernize-unary-static-assert-check
+  `_ check
+
+  The check diagnoses any ``static_assert`` declaration with an empty string literal
+  and provides a fix-it to replace the declaration with a single-argument ``static_assert`` declaration.
+
 - Improved `modernize-use-emplace
   `_ check
 
Index: clang-tidy/modernize/UnaryStaticAssertCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.h
@@ -0,0 +1,36 @@
+//===--- UnaryStaticAssertCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces a static_assert declaration with an empty message
+/// with the unary version.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-unary-static-assert.html
+class UnaryStaticAssertCheck : public ClangTidyCheck {
+public:
+  UnaryStaticAssertCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, 

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk added inline comments.



Comment at: clang-tidy/modernize/UnaryStaticAssertCheck.cpp:32
+
+  if (!AssertMessage || AssertMessage->getLength())
+return;

aaron.ballman wrote:
> I think this should be `!AssertMessage->getLength()`
It works correctly without the negation, otherwise the tests fail. 


https://reviews.llvm.org/D35257



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

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 106148.
barancsuk marked 2 inline comments as done.
barancsuk added a comment.

- Further reviews addressed


https://reviews.llvm.org/D35257

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-unary-static-assert.rst
  test/clang-tidy/modernize-unary-static-assert.cpp

Index: test/clang-tidy/modernize-unary-static-assert.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-unary-static-assert.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s modernize-unary-static-assert %t -- -- -std=c++1z
+
+#define FOO static_assert(sizeof(a) <= 15, "");
+#define MSG ""
+
+void f_textless(int a) {
+  static_assert(sizeof(a) <= 10, "");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when the string literal is an empty string [modernize-unary-static-assert]
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 10 );{{$}}
+  static_assert(sizeof(a) <= 12, L"");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 12 );{{$}}
+  FOO
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  FOO{{$}}
+  static_assert(sizeof(a) <= 17, MSG);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 17, MSG);{{$}}
+}
+
+void f_with_tex(int a) {
+  static_assert(sizeof(a) <= 10, "Size of variable a is out of range!");
+}
+
+void f_unary(int a) { static_assert(sizeof(a) <= 10); }
+
+void f_incorrect_assert() { static_assert(""); }
Index: docs/clang-tidy/checks/modernize-unary-static-assert.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-unary-static-assert.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - modernize-unary-static-assert
+
+modernize-unary-static-assert
+=
+
+The check diagnoses any ``static_assert`` declaration with an empty string literal
+and provides a fix-it to replace the declaration with a single-argument ``static_assert`` declaration.
+
+The check is only applicable for C++17 and later code.
+
+The following code:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10, "");
+  }
+
+is replaced by:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10);
+  }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -132,6 +132,7 @@
modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
+   modernize-unary-static-assert
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -108,6 +108,12 @@
   Finds and replaces explicit calls to the constructor in a return statement by
   a braced initializer list so that the return type is not needlessly repeated.
 
+- New `modernize-unary-static-assert-check
+  `_ check
+
+  The check diagnoses any ``static_assert`` declaration with an empty string literal
+  and provides a fix-it to replace the declaration with a single-argument ``static_assert`` declaration.
+
 - Improved `modernize-use-emplace
   `_ check
 
Index: clang-tidy/modernize/UnaryStaticAssertCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.h
@@ -0,0 +1,36 @@
+//===--- UnaryStaticAssertCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces a static_assert declaration with an empty message
+/// with the unary version.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-unary-static-assert.html
+class UnaryStaticAssertCheck : public 

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 106026.
barancsuk added a comment.

- Missed some fixes last time


https://reviews.llvm.org/D35257

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-unary-static-assert.rst
  test/clang-tidy/modernize-unary-static-assert.cpp

Index: test/clang-tidy/modernize-unary-static-assert.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-unary-static-assert.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s modernize-unary-static-assert %t -- -- -std=c++1z
+
+#define FOO static_assert(sizeof(a) <= 15, "");
+
+void f_textless(int a) {
+  static_assert(sizeof(a) <= 10, "");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when the string literal is an empty string [modernize-unary-static-assert]
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 10 );{{$}}
+  static_assert(sizeof(a) <= 12, L"");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 12 );{{$}}
+  FOO
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  FOO{{$}}
+}
+
+void f_with_tex(int a) {
+  static_assert(sizeof(a) <= 10, "Sie of variable a is out of range!");
+}
+
+void f_unary(int a) { static_assert(sizeof(a) <= 10); }
+
+void f_incorrect_assert() { static_assert(""); }
Index: docs/clang-tidy/checks/modernize-unary-static-assert.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-unary-static-assert.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - modernize-unary-static-assert
+
+modernize-unary-static-assert
+=
+
+The check diagnoses any ``static_assert`` declaration with an empty string literal
+and provides a fix-it to replace the declaration with a single-argument ``static_assert`` declaration.
+
+The check is only applicable for C++17 and later code.
+
+The following code:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10, "");
+  }
+
+is replaced by:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10);
+  }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -132,6 +132,7 @@
modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
+   modernize-unary-static-assert
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -108,6 +108,12 @@
   Finds and replaces explicit calls to the constructor in a return statement by
   a braced initializer list so that the return type is not needlessly repeated.
 
+- New `modernize-unary-static-assert-check
+  `_ check
+
+  The check diagnoses any ``static_assert`` declaration with an empty string literal
+  and provides a fix-it to replace the declaration with a single-argument ``static_assert`` declaration.
+
 - Improved `modernize-use-emplace
   `_ check
 
Index: clang-tidy/modernize/UnaryStaticAssertCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.h
@@ -0,0 +1,36 @@
+//===--- UnaryStaticAssertCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces a static_assert declaration with an empty message
+/// with the unary version.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-unary-static-assert.html
+class UnaryStaticAssertCheck : public ClangTidyCheck {
+public:
+  UnaryStaticAssertCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const 

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 106025.
barancsuk marked 10 inline comments as done.
barancsuk added a comment.

- Address review comments


https://reviews.llvm.org/D35257

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-unary-static-assert.rst
  test/clang-tidy/modernize-unary-static-assert.cpp

Index: test/clang-tidy/modernize-unary-static-assert.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-unary-static-assert.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s modernize-unary-static-assert %t -- -- -std=c++1z
+
+#define FOO static_assert(sizeof(a) <= 15, "");
+
+void f_textless(int a) {
+  static_assert(sizeof(a) <= 10, "");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when the string literal is an empty string [modernize-unary-static-assert]
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 10 );{{$}}
+  static_assert(sizeof(a) <= 12, L"");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 12 );{{$}}
+  FOO
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  FOO{{$}}
+}
+
+void f_with_tex(int a) {
+  static_assert(sizeof(a) <= 10, "Sie of variable a is out of range!");
+}
+
+void f_unary(int a) { static_assert(sizeof(a) <= 10); }
+
+void f_incorrect_assert() { static_assert(""); }
Index: docs/clang-tidy/checks/modernize-unary-static-assert.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-unary-static-assert.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - modernize-unary-static-assert
+
+modernize-unary-static-assert
+=
+
+The check finds ``static_assert`` declarations with empty messages, and
+replaces them with an unary ``static_assert`` declaration.
+
+The check is only applicable for C++17 code.
+
+The following code:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10, "");
+  }
+
+is replaced by:
+
+.. code-block:: c++
+
+  void f_textless(int a) {
+static_assert(sizeof(a) <= 10);
+  }
+
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -132,6 +132,7 @@
modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
+   modernize-unary-static-assert
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -108,6 +108,11 @@
   Finds and replaces explicit calls to the constructor in a return statement by
   a braced initializer list so that the return type is not needlessly repeated.
 
+- New `modernize-unary-static-assert-check
+  `_ check
+
+  The check finds ``static_assert`` declarations with empty messages, and replaces them with an unary ``static_assert`` declaration.
+
 - Improved `modernize-use-emplace
   `_ check
 
Index: clang-tidy/modernize/UnaryStaticAssertCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.h
@@ -0,0 +1,36 @@
+//===--- UnaryStaticAssertCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces a static_assert declaration with an empty message
+/// with the unary version.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-unary-static-assert.html
+class UnaryStaticAssertCheck : public ClangTidyCheck {
+public:
+  UnaryStaticAssertCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace modernize
+} // namespace 

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk updated this revision to Diff 106021.
barancsuk added a comment.

- Do not rewrite macro expansions


https://reviews.llvm.org/D35257

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-unary-static-assert.rst
  test/clang-tidy/modernize-unary-static-assert.cpp

Index: test/clang-tidy/modernize-unary-static-assert.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-unary-static-assert.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy %s modernize-unary-static-assert %t -- -- -std=c++1z
+
+#define FOO static_assert(sizeof(a) <= 15, "");
+
+void f_textless(int a) {
+  static_assert(sizeof(a) <= 10, "");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when the assert message is unused [modernize-unary-static-assert]
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 10 );{{$}}
+  FOO
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when
+  // CHECK-FIXES: {{^}}  FOO{{$}}
+}
+
+void f_with_tex(int a) {
+  static_assert(sizeof(a) <= 10, "Sie of variable a is out of range!");
+}
+
+void f_unary(int a) { static_assert(sizeof(a) <= 10); }
+
+void f_incorrect_assert() { static_assert(""); }
Index: docs/clang-tidy/checks/modernize-unary-static-assert.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-unary-static-assert.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - modernize-unary-static-assert
+
+modernize-unary-static-assert
+=
+
+The check finds ``static_assert`` declarations with empty messages, and
+replaces them with an unary ``static_assert`` declaration.
+
+The check is only applicable for C++17 code.
+
+The following code:
+
+.. code-block:: c++
+
+  void f_textless(int a){
+static_assert(sizeof(a) <= 10, "");
+  }
+
+is replaced by:
+
+.. code-block:: c++
+
+  void f_textless(int a){
+static_assert(sizeof(a) <= 10 );
+  }
+
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -132,6 +132,7 @@
modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
+   modernize-unary-static-assert
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
Index: clang-tidy/modernize/UnaryStaticAssertCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.h
@@ -0,0 +1,36 @@
+//===--- UnaryStaticAssertCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces static assert declaration with empty message
+/// with their unary version.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-unary-static-assert.html
+class UnaryStaticAssertCheck : public ClangTidyCheck {
+public:
+  UnaryStaticAssertCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
Index: clang-tidy/modernize/UnaryStaticAssertCheck.cpp
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.cpp
@@ -0,0 +1,50 @@
+//===--- UnaryStaticAssertCheck.cpp - clang-tidy---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "UnaryStaticAssertCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+void UnaryStaticAssertCheck::registerMatchers(MatchFinder *Finder) {
+  if 

[PATCH] D35257: [clang-tidy] Add new modernize use unary assert check

[Barancsuk Lilla via Phabricator via cfe-commits]

barancsuk created this revision.
barancsuk added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, JDevlieghere, mgorny.

The check finds `static_assert` declarations with empty messages, and
 replaces them with an unary `static_assert` declaration.

The check is only applicable for C++17 code.

The following code:

  void f_textless(int a){
static_assert(sizeof(a) <= 10, "");
  }
   

is replaced by:

  void f_textless(int a){
static_assert(sizeof(a) <= 10 );
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D35257

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.cpp
  clang-tidy/modernize/UnaryStaticAssertCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-unary-static-assert.rst
  test/clang-tidy/modernize-unary-static-assert.cpp

Index: test/clang-tidy/modernize-unary-static-assert.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-unary-static-assert.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s modernize-unary-static-assert %t -- -- -std=c++1z
+
+void f_textless(int a) {
+  static_assert(sizeof(a) <= 10, "");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use unary 'static_assert' when the assert message is unused [modernize-unary-static-assert]
+  // CHECK-FIXES: {{^}}  static_assert(sizeof(a) <= 10 );{{$}}
+}
+
+void f_with_tex(int a) {
+  static_assert(sizeof(a) <= 10, "Sie of variable a is out of range!");
+}
+
+void f_unary(int a) { static_assert(sizeof(a) <= 10); }
+
+void f_incorrect_assert() { static_assert(""); }
Index: docs/clang-tidy/checks/modernize-unary-static-assert.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-unary-static-assert.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - modernize-unary-static-assert
+
+modernize-unary-static-assert
+=
+
+The check finds ``static_assert`` declarations with empty messages, and
+replaces them with an unary ``static_assert`` declaration.
+
+The check is only applicable for C++17 code.
+
+The following code:
+
+.. code-block:: c++
+
+  void f_textless(int a){
+static_assert(sizeof(a) <= 10, "");
+  }
+
+is replaced by:
+
+.. code-block:: c++
+
+  void f_textless(int a){
+static_assert(sizeof(a) <= 10 );
+  }
+
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -132,6 +132,7 @@
modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
+   modernize-unary-static-assert
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
Index: clang-tidy/modernize/UnaryStaticAssertCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.h
@@ -0,0 +1,36 @@
+//===--- UnaryStaticAssertCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replaces static assert declaration with empty message
+/// with their unary version.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-unary-static-assert.html
+class UnaryStaticAssertCheck : public ClangTidyCheck {
+public:
+  UnaryStaticAssertCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_UNARY_STATIC_ASSERT_H
Index: clang-tidy/modernize/UnaryStaticAssertCheck.cpp
===
--- /dev/null
+++ clang-tidy/modernize/UnaryStaticAssertCheck.cpp
@@ -0,0 +1,46 @@
+//===--- UnaryStaticAssertCheck.cpp - clang-tidy---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "UnaryStaticAssertCheck.h"
+#include "clang/AST/ASTContext.h"
+#include