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

[Douglas Gregor]

On Jun 2, 2011, at 7:40 AM, Hans Wennborg wrote:

> 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.

I like this a lot better. It'll work cleanly for both C and C++, thanks!

>> 
>> 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;


This does look like a bug :)

Patch approved, thanks!

	- Doug