[PATCH] D81751: [SemaObjC] Fix a -Wobjc-signed-char-bool false-positive with binary conditional operator

[Erik Pilkington via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2f71cf6d77c5: [SemaObjC] Fix a -Wobjc-signed-char-bool 
false-positive with binary conditional… (authored by erik.pilkington).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D81751?vs=270455=275675#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81751

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/SemaObjC/signed-char-bool-conversion.m


Index: clang/test/SemaObjC/signed-char-bool-conversion.m
===
--- clang/test/SemaObjC/signed-char-bool-conversion.m
+++ clang/test/SemaObjC/signed-char-bool-conversion.m
@@ -108,3 +108,15 @@
   f(); // expected-note {{in instantiation of function template 
specialization 'f' requested here}}
 }
 #endif
+
+void t5(BOOL b) {
+  int i;
+  b = b ?: YES; // no warning
+  b = b ?: i; // expected-warning {{implicit conversion from integral type 
'int' to 'BOOL'}}
+  b = (b = i) // expected-warning {{implicit conversion from integral type 
'int' to 'BOOL'}}
+   ?: YES;
+  b = (1 ? YES : i) ?: YES; // expected-warning {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+  b = b ?: (1 ? i : i); // expected-warning 2 {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+
+  b = b ? YES : (i ?: 0); // expected-warning {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11936,27 +11936,31 @@
   }
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
  SourceLocation CC, QualType T);
 
 static void CheckConditionalOperand(Sema , Expr *E, QualType T,
 SourceLocation CC, bool ) {
   E = E->IgnoreParenImpCasts();
 
-  if (isa(E))
-return CheckConditionalOperator(S, cast(E), CC, T);
+  if (auto *CO = dyn_cast(E))
+return CheckConditionalOperator(S, CO, CC, T);
 
   AnalyzeImplicitConversions(S, E, CC);
   if (E->getType() != T)
 return CheckImplicitConversion(S, E, T, CC, );
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
  SourceLocation CC, QualType T) {
   AnalyzeImplicitConversions(S, E->getCond(), E->getQuestionLoc());
 
+  Expr *TrueExpr = E->getTrueExpr();
+  if (auto *BCO = dyn_cast(E))
+TrueExpr = BCO->getCommon();
+
   bool Suspicious = false;
-  CheckConditionalOperand(S, E->getTrueExpr(), T, CC, Suspicious);
+  CheckConditionalOperand(S, TrueExpr, T, CC, Suspicious);
   CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious);
 
   if (T->isBooleanType())
@@ -11975,7 +11979,7 @@
   if (E->getType() == T) return;
 
   Suspicious = false;
-  CheckImplicitConversion(S, E->getTrueExpr()->IgnoreParenImpCasts(),
+  CheckImplicitConversion(S, TrueExpr->IgnoreParenImpCasts(),
   E->getType(), CC, );
   if (!Suspicious)
 CheckImplicitConversion(S, E->getFalseExpr()->IgnoreParenImpCasts(),
@@ -12038,7 +12042,7 @@
 
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
-  if (auto *CO = dyn_cast(SourceExpr)) {
+  if (auto *CO = dyn_cast(SourceExpr)) {
 CheckConditionalOperator(S, CO, CC, T);
 return;
   }


Index: clang/test/SemaObjC/signed-char-bool-conversion.m
===
--- clang/test/SemaObjC/signed-char-bool-conversion.m
+++ clang/test/SemaObjC/signed-char-bool-conversion.m
@@ -108,3 +108,15 @@
   f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
 }
 #endif
+
+void t5(BOOL b) {
+  int i;
+  b = b ?: YES; // no warning
+  b = b ?: i; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  b = (b = i) // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+   ?: YES;
+  b = (1 ? YES : i) ?: YES; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  b = b ?: (1 ? i : i); // expected-warning 2 {{implicit conversion from integral type 'int' to 'BOOL'}}
+
+  b = b ? YES : (i ?: 0); // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11936,27 +11936,31 @@
   }
 }
 
-static void 

[PATCH] D81751: [SemaObjC] Fix a -Wobjc-signed-char-bool false-positive with binary conditional operator

[Erik Pilkington via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2f71cf6d77c5: [SemaObjC] Fix a -Wobjc-signed-char-bool 
false-positive with binary conditional… (authored by erik.pilkington).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D81751?vs=270455=276130#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81751

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/SemaObjC/signed-char-bool-conversion.m


Index: clang/test/SemaObjC/signed-char-bool-conversion.m
===
--- clang/test/SemaObjC/signed-char-bool-conversion.m
+++ clang/test/SemaObjC/signed-char-bool-conversion.m
@@ -108,3 +108,15 @@
   f(); // expected-note {{in instantiation of function template 
specialization 'f' requested here}}
 }
 #endif
+
+void t5(BOOL b) {
+  int i;
+  b = b ?: YES; // no warning
+  b = b ?: i; // expected-warning {{implicit conversion from integral type 
'int' to 'BOOL'}}
+  b = (b = i) // expected-warning {{implicit conversion from integral type 
'int' to 'BOOL'}}
+   ?: YES;
+  b = (1 ? YES : i) ?: YES; // expected-warning {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+  b = b ?: (1 ? i : i); // expected-warning 2 {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+
+  b = b ? YES : (i ?: 0); // expected-warning {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11936,27 +11936,31 @@
   }
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
  SourceLocation CC, QualType T);
 
 static void CheckConditionalOperand(Sema , Expr *E, QualType T,
 SourceLocation CC, bool ) {
   E = E->IgnoreParenImpCasts();
 
-  if (isa(E))
-return CheckConditionalOperator(S, cast(E), CC, T);
+  if (auto *CO = dyn_cast(E))
+return CheckConditionalOperator(S, CO, CC, T);
 
   AnalyzeImplicitConversions(S, E, CC);
   if (E->getType() != T)
 return CheckImplicitConversion(S, E, T, CC, );
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
  SourceLocation CC, QualType T) {
   AnalyzeImplicitConversions(S, E->getCond(), E->getQuestionLoc());
 
+  Expr *TrueExpr = E->getTrueExpr();
+  if (auto *BCO = dyn_cast(E))
+TrueExpr = BCO->getCommon();
+
   bool Suspicious = false;
-  CheckConditionalOperand(S, E->getTrueExpr(), T, CC, Suspicious);
+  CheckConditionalOperand(S, TrueExpr, T, CC, Suspicious);
   CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious);
 
   if (T->isBooleanType())
@@ -11975,7 +11979,7 @@
   if (E->getType() == T) return;
 
   Suspicious = false;
-  CheckImplicitConversion(S, E->getTrueExpr()->IgnoreParenImpCasts(),
+  CheckImplicitConversion(S, TrueExpr->IgnoreParenImpCasts(),
   E->getType(), CC, );
   if (!Suspicious)
 CheckImplicitConversion(S, E->getFalseExpr()->IgnoreParenImpCasts(),
@@ -12038,7 +12042,7 @@
 
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
-  if (auto *CO = dyn_cast(SourceExpr)) {
+  if (auto *CO = dyn_cast(SourceExpr)) {
 CheckConditionalOperator(S, CO, CC, T);
 return;
   }


Index: clang/test/SemaObjC/signed-char-bool-conversion.m
===
--- clang/test/SemaObjC/signed-char-bool-conversion.m
+++ clang/test/SemaObjC/signed-char-bool-conversion.m
@@ -108,3 +108,15 @@
   f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
 }
 #endif
+
+void t5(BOOL b) {
+  int i;
+  b = b ?: YES; // no warning
+  b = b ?: i; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  b = (b = i) // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+   ?: YES;
+  b = (1 ? YES : i) ?: YES; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  b = b ?: (1 ? i : i); // expected-warning 2 {{implicit conversion from integral type 'int' to 'BOOL'}}
+
+  b = b ? YES : (i ?: 0); // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11936,27 +11936,31 @@
   }
 }
 
-static void 

[PATCH] D81751: [SemaObjC] Fix a -Wobjc-signed-char-bool false-positive with binary conditional operator

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

This looks good to me.




Comment at: clang/lib/Sema/SemaChecking.cpp:11825
 
+  Expr *TrueExpr = E->getTrueExpr();
+  if (isa(E))

Can you use `BinaryConditionalOperator::getCommon` here?


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

https://reviews.llvm.org/D81751



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

[PATCH] D81751: [SemaObjC] Fix a -Wobjc-signed-char-bool false-positive with binary conditional operator

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a reviewer: ahatanak.
erik.pilkington added a comment.

Ping!


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

https://reviews.llvm.org/D81751



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

[PATCH] D81751: [SemaObjC] Fix a -Wobjc-signed-char-bool false-positive with binary conditional operator

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.
erik.pilkington added a reviewer: rjmccall.
Herald added subscribers: ributzka, dexonsmith, jkorous.

We were previously bypassing the conditional expression special case for binary 
conditional expressions. Also, dig through the OpaqueValueExpr on the left of 
the ?:.

rdar://64134411


https://reviews.llvm.org/D81751

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/SemaObjC/signed-char-bool-conversion.m


Index: clang/test/SemaObjC/signed-char-bool-conversion.m
===
--- clang/test/SemaObjC/signed-char-bool-conversion.m
+++ clang/test/SemaObjC/signed-char-bool-conversion.m
@@ -108,3 +108,15 @@
   f(); // expected-note {{in instantiation of function template 
specialization 'f' requested here}}
 }
 #endif
+
+void t5(BOOL b) {
+  int i;
+  b = b ?: YES; // no warning
+  b = b ?: i; // expected-warning {{implicit conversion from integral type 
'int' to 'BOOL'}}
+  b = (b = i) // expected-warning {{implicit conversion from integral type 
'int' to 'BOOL'}}
+   ?: YES;
+  b = (1 ? YES : i) ?: YES; // expected-warning {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+  b = b ?: (1 ? i : i); // expected-warning 2 {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+
+  b = b ? YES : (i ?: 0); // expected-warning {{implicit conversion from 
integral type 'int' to 'BOOL'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11803,27 +11803,33 @@
   }
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
  SourceLocation CC, QualType T);
 
 static void CheckConditionalOperand(Sema , Expr *E, QualType T,
 SourceLocation CC, bool ) {
   E = E->IgnoreParenImpCasts();
 
-  if (isa(E))
-return CheckConditionalOperator(S, cast(E), CC, T);
+  if (auto *CO = dyn_cast(E))
+return CheckConditionalOperator(S, CO, CC, T);
 
   AnalyzeImplicitConversions(S, E, CC);
   if (E->getType() != T)
 return CheckImplicitConversion(S, E, T, CC, );
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
  SourceLocation CC, QualType T) {
   AnalyzeImplicitConversions(S, E->getCond(), E->getQuestionLoc());
 
+  Expr *TrueExpr = E->getTrueExpr();
+  if (isa(E))
+if (auto *OVE = dyn_cast(TrueExpr))
+  if (Expr *SourceExpr = OVE->getSourceExpr())
+TrueExpr = SourceExpr;
+
   bool Suspicious = false;
-  CheckConditionalOperand(S, E->getTrueExpr(), T, CC, Suspicious);
+  CheckConditionalOperand(S, TrueExpr, T, CC, Suspicious);
   CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious);
 
   if (T->isBooleanType())
@@ -11842,7 +11848,7 @@
   if (E->getType() == T) return;
 
   Suspicious = false;
-  CheckImplicitConversion(S, E->getTrueExpr()->IgnoreParenImpCasts(),
+  CheckImplicitConversion(S, TrueExpr->IgnoreParenImpCasts(),
   E->getType(), CC, );
   if (!Suspicious)
 CheckImplicitConversion(S, E->getFalseExpr()->IgnoreParenImpCasts(),
@@ -11905,7 +11911,7 @@
 
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
-  if (auto *CO = dyn_cast(SourceExpr)) {
+  if (auto *CO = dyn_cast(SourceExpr)) {
 CheckConditionalOperator(S, CO, CC, T);
 return;
   }


Index: clang/test/SemaObjC/signed-char-bool-conversion.m
===
--- clang/test/SemaObjC/signed-char-bool-conversion.m
+++ clang/test/SemaObjC/signed-char-bool-conversion.m
@@ -108,3 +108,15 @@
   f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
 }
 #endif
+
+void t5(BOOL b) {
+  int i;
+  b = b ?: YES; // no warning
+  b = b ?: i; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  b = (b = i) // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+   ?: YES;
+  b = (1 ? YES : i) ?: YES; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  b = b ?: (1 ? i : i); // expected-warning 2 {{implicit conversion from integral type 'int' to 'BOOL'}}
+
+  b = b ? YES : (i ?: 0); // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11803,27 +11803,33 @@
   }
 }
 
-static void CheckConditionalOperator(Sema , ConditionalOperator *E,
+static void CheckConditionalOperator(Sema ,