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

[Douglas Gregor]

On Jun 1, 2011, at 5:52 AM, Hans Wennborg wrote:

> This is an attempt to address some of http://llvm.org/bugs/show_bug.cgi?id=9969
> 
> The idea is to warn about code such as:
> 
> int foo(int x, bool someCondition) {
> return x + someCondition ? 42 : 0;
> }
> 
> where it seems likely that the programmer forgot that ?: has lower
> precedence than +.
> 
> The patch looks for condition expressions which are non-boolean binary
> operators, implicitly cast to bool, where the right-hand side
> expression is bool.


Cool. A few comments:

+/// 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?

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

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

	- Doug