Page MenuHomePhabricator

[AST] Fixed extraneous warnings for binary conditional operator
ClosedPublic

Authored by Nathan-Huckleberry on Jun 14 2019, 3:57 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 3:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
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 accepted this revision.Jun 14 2019, 4:58 PM
rsmith added inline comments.
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?

This revision is now accepted and ready to land.Jun 14 2019, 4:58 PM
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.

  • [AST] Cleanup of conditional operator unused warning detection
clang/lib/AST/Expr.cpp
2348 ↗(On Diff #205117)

const auto *Exp

2353 ↗(On Diff #205117)

const auto *Exp

  • [AST] Formatting changes to ConditionalOperator unused detection
nickdesaulniers accepted this revision.Jun 17 2019, 11:28 AM

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

  • [analyzer] Fix clang-tidy crash on GCCAsmStmt

Mistakenly updated revision. Attempting to revert.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 11:34 AM