[cfe-commits] [Patch] Warn about missing parentheses for conditional operator

[Hans Wennborg]

On Wed, Jun 1, 2011 at 11:31 PM, Douglas Gregor <dgregor at apple.com> wrote:
> +/// DiagnoseConditionalPrecedence - Emit a warning when a conditional operator
> +/// and binary operator are mixed in a way that suggests the programmer assumed
> +/// the conditional operator has higher precedence, for example:
> +/// "int x = a + someBinaryCondition ? 1 : 2".
> +static void DiagnoseConditionalPrecedence(Sema &Self,
> +                                          SourceLocation OpLoc,
> +                                          Expr *cond,
> +                                          Expr *lhs,
> +                                          Expr *rhs) {
> +  if (ImplicitCastExpr *CE = dyn_cast<ImplicitCastExpr>(cond)) {
> +    if (BinaryOperator* OP = dyn_cast<BinaryOperator>(CE->getSubExpr())) {
> +      if (OP->getType()->isBooleanType())
> +        return;
>
> This isn't going to work in C, because comparison operators in C have type 'int'. Rather than looking at the type of the BinaryOperator, why not look for specific operators (+, -, etc.) that are likely to be misunderstood?

I tried just looking for arithmetic operators (+,-,*,/,%), but then we
warn on code such as:

    TextDirection direction() const { return m_bidiEmbeddingLevel % 2
? RTL : LTR; }

I suppose we could just exclude the % operator, but then we miss out
on warning for stuff like

    int x = y % (foo==bar) ? 16 : 32;

So I think the trick to getting this warning tight is to try and
figure out if the right-hand side of the condition expression looks
boolean.

Attaching a new patch that tries to do this.

>
> Also, it looks like your patch is missing the required changes to DiagnosticSemaKinds.td.

Oops, adding that.

>
> Finally, did you try running this patch over (say) the LLVM + Clang code base, to evaluate its signal/noise ratio?

This warning doesn't find anything in LLVM/Clang. In Chromium it finds
this, which looks like a bug :)

    gmt = non_gmt + (zone[0] == '+') ? offset : -offset;



Thanks for your input,
Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_conditional_precedence_2.diff
Type: text/x-patch
Size: 11150 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110602/301924b8/attachment.bin>