[AST] Fixed extraneous warnings for binary conditional operator

[Nathan-Huckleberry]

Summary

Binary conditional operator gave warnings where ternary operators
did not. They have been fixed to warn similarly to ternary operators.

Link: https://bugs.llvm.org/show_bug.cgi?id=42239

[nickdesaulniers]

clang/test/Sema/warn-binary-conditional-expression-unused.c

8 ↗

(On Diff #204871)

Note to other reviewers:

Whether or not we should warn on a here is another story and orthogonal to this patch, which focuses on just fixing existing inconsistency between ternary and gnu binary conditional that exists today.

[rsmith]

clang/lib/AST/Expr.cpp

2351–2352 ↗

(On Diff #204871)

Looks like whoever wrote this expected to handle binary conditional operators here. Maybe delete this check since it's impossible for a ternary conditional operator?

[nickdesaulniers]

clang/lib/AST/Expr.cpp

2351–2352 ↗

(On Diff #204871)

I agree; I assume it's not possible for this case and that they meant to handle BinaryConditionalOperatorClass. This should make the the return value a simple logical and of the two sides; which more closely follows the comment above.

[Nathan-Huckleberry]

• [AST] Cleanup of conditional operator unused warning detection

[nickdesaulniers]

clang/lib/AST/Expr.cpp

2348 ↗

(On Diff #205117)

const auto *Exp

2353 ↗

(On Diff #205117)

const auto *Exp

[Nathan-Huckleberry]

• [AST] Formatting changes to ConditionalOperator unused detection

[nickdesaulniers]

Thanks for the patch and following up on the review comments. Let's work on getting you commit access now.

[Nathan-Huckleberry]

• [analyzer] Fix clang-tidy crash on GCCAsmStmt

[Nathan-Huckleberry]

Mistakenly updated revision. Attempting to revert.

cfe/trunk/lib/AST/Expr.cpp

bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
   case VAArgExprClass:
   case AtomicExprClass:
     return false;
 
   case ConditionalOperatorClass: {
     // If only one of the LHS or RHS is a warning, the operator might
     // be being used for control flow. Only warn if both the LHS and
     // RHS are warnings.
-    const ConditionalOperator *Exp = cast<ConditionalOperator>(this);
-    if (!Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx))
-      return false;
-    if (!Exp->getLHS())
-      return true;
-    return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+    const auto *Exp = cast<ConditionalOperator>(this);
+    return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx) &&
+           Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+  }
+  case BinaryConditionalOperatorClass: {
+    const auto *Exp = cast<BinaryConditionalOperator>(this);
+    return Exp->getFalseExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
   }
 
   case MemberExprClass:
     WarnE = this;
     Loc = cast<MemberExpr>(this)->getMemberLoc();
     R1 = SourceRange(Loc, Loc);
     R2 = cast<MemberExpr>(this)->getBase()->getSourceRange();
     return true;

cfe/trunk/test/Sema/warn-binary-conditional-expression-unused.c

+// RUN: %clang_cc1 -fsyntax-only -Wunused-value -verify %s
+int main() {
+    int a;
+    int b;
+    a ? : b; //expected-warning{{expression result unused}}
+    a ? a : b; //expected-warning{{expression result unused}}
+    a ? : ++b;
+    a ? a : ++b;
+    ++a ? : b; //expected-warning{{expression result unused}}
+    ++a ? a : b; //expected-warning{{expression result unused}}
+    ++a ? : ++b;
+    ++a ? a : ++b;
+    return 0;
+};
+