[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-03 Thread eric via Phabricator via cfe-commits 
Higuoxing created this revision.
Herald added a subscriber: cfe-commits.
Higuoxing edited the summary of this revision.

Hi,

As you see, to avoid some expression like

  assert(x || y && "some messages");

clang will skip all the parentheses check if the expression is in `macro`;
and this rule is a little bit loose, if a function which takes boolean 
variables defined in `macros`,
clang will not check parentheses for these functions;

So, in this patch I add a string type checking, if and only if a function 
defined in macro and the RHS of boolean expression is a string, then skip the 
parentheses checking :)

Thanks for reviewing :)


Repository:
  rC Clang

https://reviews.llvm.org/D47687

Files:
  lib/Sema/SemaExpr.cpp


Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12310,7 +12310,14 @@
 
   // Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
   // We don't warn for 'assert(a || b && "bad")' since this is safe.
-  if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
+  StringLiteral* diagnoseRHSToBeString
+= dyn_cast(RHSExpr->IgnoreImpCasts());
+  if (Opc == BO_LOr && 
+ !(OpLoc.isMacroID() && 
+ diagnoseRHSToBeString)
+/* Don't warn in macros, 
+   if and only if RHS could be 
+   evaluated as string */) {
 DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
 DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
   }


Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12310,7 +12310,14 @@
 
   // Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
   // We don't warn for 'assert(a || b && "bad")' since this is safe.
-  if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
+  StringLiteral* diagnoseRHSToBeString
+= dyn_cast(RHSExpr->IgnoreImpCasts());
+  if (Opc == BO_LOr && 
+ !(OpLoc.isMacroID() && 
+ diagnoseRHSToBeString)
+/* Don't warn in macros, 
+   if and only if RHS could be 
+   evaluated as string */) {
 DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
 DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-03 Thread eric via Phabricator via cfe-commits 
Higuoxing added a comment.

As for some test cases,

  $ cat a.cc
  
  #include 
  #include 
  
  #define bar(x) \
( \
  ( std::cout << x ) \
)
  
  bool x;
  int val;
  
  void foo(bool b) {
std::cout << b << std::endl;
  }
  
  int main () {
  
  
foo(x && val == 4 || (!x && val == 5));
bar(x && val == 4 || (!x && val == 5));
assert(x && val == 4 || (!x && val == 5));
assert(x || val == 4 && "test");
return 0;
  }

And clang will emits

  a.cc:20:9: warning: '&&' within '||' [-Wlogical-op-parentheses]
foo(x && val == 4 || (!x && val == 5));
~~^~~ ~~
  a.cc:20:9: note: place parentheses around the '&&' expression to silence this 
warning
foo(x && val == 4 || (!x && val == 5));
  ^
()
  a.cc:21:9: warning: '&&' within '||' [-Wlogical-op-parentheses]
bar(x && val == 4 || (!x && val == 5));
~~^~~~
  a.cc:7:20: note: expanded from macro 'bar'
  ( std::cout << x ) \
~^
  a.cc:21:9: note: place parentheses around the '&&' expression to silence this 
warning
bar(x && val == 4 || (!x && val == 5));
~~^~~~
  a.cc:7:20: note: expanded from macro 'bar'
  ( std::cout << x ) \
~^
  a.cc:22:12: warning: '&&' within '||' [-Wlogical-op-parentheses]
assert(x && val == 4 || (!x && val == 5));
   ~~^~~ ~~
  /usr/include/assert.h:93:25: note: expanded from macro 'assert'
  (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, 
#e) : (void)0)
  ^
  a.cc:22:12: note: place parentheses around the '&&' expression to silence 
this warning
assert(x && val == 4 || (!x && val == 5));
   ~~^~~
  /usr/include/assert.h:93:25: note: expanded from macro 'assert'
  (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, 
#e) : (void)0)
  ^
  3 warnings generated.


Repository:
  rC Clang

https://reviews.llvm.org/D47687



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

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-03 Thread eric via Phabricator via cfe-commits 
Higuoxing added a comment.

In https://reviews.llvm.org/D47687#1120226, @lebedev.ri wrote:

> In https://reviews.llvm.org/D47687#1120213, @Higuoxing wrote:
>
> > As for some test cases,
>
>
> The tests need to be within the patch itself, in this case
>  i guess that should go into `clang/test/Sema/`.
>  See other tests in there on how to write them.
>  And they will be run via `$ ninja check-clang` /  `$ ninja check-clang-sema`.


Hi,

Thanks for your reviewing :D I am trying to write the test cases ...

Thanks :D


Repository:
  rC Clang

https://reviews.llvm.org/D47687



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

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-04 Thread eric via Phabricator via cfe-commits 
Higuoxing updated this revision to Diff 149766.
Higuoxing edited the summary of this revision.
Higuoxing added a comment.

Update with test cases :)


https://reviews.llvm.org/D47687

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.c


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,11 @@
   if ((i = 4)) {}
 }
 
+#define macro_parentheses_check(x) \
+  ( \
+( (void)(x) ) \
+  )
+
 void bitwise_rel(unsigned i) {
   (void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} 
\
 // expected-note{{place parentheses around the '==' 
expression to silence this warning}} \
@@ -96,6 +101,23 @@
 
   (void)(i && i || 0); // no warning.
   (void)(0 || i && i); // no warning.
+
+  macro_parentheses_check(i || i && i); // expected-warning {{'&&' within 
'||'}} \
+// expected-note {{place parentheses 
around the '&&' expression to silence this warning}}
+  
+  macro_parentheses_check(i || i && "I love LLVM"); // no warning.
+  macro_parentheses_check("I love LLVM" && i || i); // no warning.
+
+  macro_parentheses_check(i || i && "I love LLVM" || i); // expected-warning 
{{'&&' within '||'}} \
+ // expected-note 
{{place parentheses around the '&&' expression to silence this warning}}
+  
+
+  macro_parentheses_check(i || "I love LLVM" && i || i); // expected-warning 
{{'&&' within '||'}} \
+ // expected-note 
{{place parentheses around the '&&' expression to silence this warning}}
+  
+
+  macro_parentheses_check(i && i || 0); // no warning.
+  macro_parentheses_check(0 || i && i); // no warning.
 }
 
 _Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12205,6 +12205,7 @@
   if (EvaluatesAsFalse(S, RHSExpr))
 return;
   // If it's "1 && a || b" don't warn since the precedence doesn't matter.
+  // And 'assert("some message" && a || b)' don't warn as well.
   if (!EvaluatesAsTrue(S, Bop->getLHS()))
 return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
 } else if (Bop->getOpcode() == BO_LOr) {
@@ -12227,6 +12228,7 @@
   if (EvaluatesAsFalse(S, LHSExpr))
 return;
   // If it's "a || b && 1" don't warn since the precedence doesn't matter.
+  // And 'assert(a || b && "some message")' don't warn as well.
   if (!EvaluatesAsTrue(S, Bop->getRHS()))
 return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
 }
@@ -12309,8 +12311,7 @@
   }
 
   // Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
-  // We don't warn for 'assert(a || b && "bad")' since this is safe.
-  if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
+  if (Opc == BO_LOr) {
 DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
 DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
   }


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,11 @@
   if ((i = 4)) {}
 }
 
+#define macro_parentheses_check(x) \
+  ( \
+( (void)(x) ) \
+  )
+
 void bitwise_rel(unsigned i) {
   (void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} \
 // expected-note{{place parentheses around the '==' expression to silence this warning}} \
@@ -96,6 +101,23 @@
 
   (void)(i && i || 0); // no warning.
   (void)(0 || i && i); // no warning.
+
+  macro_parentheses_check(i || i && i); // expected-warning {{'&&' within '||'}} \
+// expected-note {{place parentheses around the '&&' expression to silence this warning}}
+  
+  macro_parentheses_check(i || i && "I love LLVM"); // no warning.
+  macro_parentheses_check("I love LLVM" && i || i); // no warning.
+
+  macro_parentheses_check(i || i && "I love LLVM" || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+  
+
+  macro_parentheses_check(i || "I love LLVM" && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+  
+
+  macro_parentheses_check(i && i || 0); // no warning.
+  macro_parentheses_check(0 || i && i); // no warning.
 }
 
 _Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12205,6 +12205,7 @@
   if (EvaluatesAsFalse(S, RHSExpr))

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-04 Thread eric via Phabricator via cfe-commits 
Higuoxing added a comment.

Thanks for reviewing!

I think the logic that *do not emit warning in macros* is not so proper ...

Best Regards,
Xing


https://reviews.llvm.org/D47687



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

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

2018-06-06 Thread eric via Phabricator via cfe-commits 
Higuoxing added a comment.

Thanks for reviewing! I think enabling parentheses-checking in macros could be 
helpful, and `assert(something)` is widely used : )


https://reviews.llvm.org/D47687



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