[cfe-commits] [PATCH] Warning for suspicious implicit conversions from floating point to bool

[Hans Wennborg]

On Thu, Aug 23, 2012 at 2:48 PM, Andreas Eckleder <aeckleder at google.com> wrote:
> Hi Hans,
>
> Thank you very much for the review. I have reworked my code according to
> your suggestions.

Thanks! I think it's much clearer now.

I noticed that there seems to be trailing whitespace on some of the
lines. The rest of my comments are inline below.

> +++ b/include/clang/Basic/DiagnosticGroups.td
> +def ImplicitConversionFloatingPointToBool : DiagGroup<"implicit-conversion-floating-point-to-bool">;

I see that there are other lines in this file that are 80+ columns,
but I also see some like this that are wrapped, e.g.
ImplicitFallthroughPerFunction, so I think wrapping this line after
the ':' would better.

> +++ b/lib/Sema/SemaChecking.cpp
> +static bool IsImplicitBoolFloatConversion(Sema &S, Expr *Ex, bool ToBool) {

I'm not super happy about the ToBool parameter here and the logic
where it's used in the function below. I was thinking that if the
function could just take the source and target builtin types as
parameters, the function could be written as:

 - Check that the expression is a cast
 - Get the QualTypes for the outer and inner expressions
 - return TypeA->isSpecificBuiltinType(SrcTy) &&
TypeB->isSpecificBuiltinType(DstTy);  (I don't think you need to check
that they're builtins first; i believe isSpecificBuiltinType will
return false if they're not.)


> +  Expr *InnerE = Ex->IgnoreParenImpCasts();
> +  const Type *Target = S.Context.getCanonicalType(Ex->getType()).getTypePtr();
> +  const Type *Source =
> +    S.Context.getCanonicalType(InnerE->getType()).getTypePtr();

I don't think getTypePtr() is needed; working with the QualType
objects should work just as well.

> +  if (Target == Source)

Isn't this check redundant based on the code below?

> +  const BuiltinType *FloatCandidateBT =
> +    dyn_cast<BuiltinType>(ToBool ? Source : Target);
> +  const Type *BoolCandidateType = ToBool ? Target : Source;

This is the part that I find confusing.

> +void CheckImplicitArgumentConversions(Sema &S, CallExpr *TheCall,
> +                                      SourceLocation CC) {
> +  unsigned NumArgs = TheCall->getNumArgs();
> +  for (unsigned i = 0; i < NumArgs; ++i) {
> +    Expr *CurrA = TheCall->getArg(i);
> +    if (!isa<ImplicitCastExpr>(CurrA))

This if statement is redundant since you have the same check inside
IsImplicitBoolFloatConversion.

> +    bool IsSwapped = ((i > 0) &&
> +      IsImplicitBoolFloatConversion(S,TheCall->getArg(i - 1), false));

I think this should be indented two spaces more, and there should be a
space after the S argument. The same applies below.

> +    IsSwapped |= ((i < (NumArgs - 1)) &&
> +      IsImplicitBoolFloatConversion(S,TheCall->getArg(i + 1), false));
> +    if (IsSwapped)

The lack of braces around the if statement makes me nervous even
though I know that it should work :)

> +      // Warn on this floating-point to bool conversion
> +      DiagnoseImpCast(S, CurrA->IgnoreParenImpCasts(),
> +                      CurrA->getType(), CC,
> +                      diag::warn_impcast_floating_point_to_bool);
> +  }
> +}
> +

> @@ -4488,6 +4531,26 @@ void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
>        }
>      }
>
> +    // If the target is bool, warn if expr is a function or method call

I like periods at the end of comments. This applies below as well. And
I don't think there's any need to distinguish between function and
method calls; just call expression is fine.

> +    if (Target->isSpecificBuiltinType(BuiltinType::Bool) &&
> +        isa<CallExpr>(E)) {
> +      // Check last argument of function or method call to see if it is an
> +      // implicit cast from a type matching the type the method result
> +      // is being cast to
> +      CallExpr *CEx = static_cast<CallExpr*>(E);

I think this static_cast<> should be a cast<>. See
http://llvm.org/docs/ProgrammersManual.html#isa

> +  // Check implicit argument conversions for function calls
> +  if (isa<CallExpr>(E))
> +    CheckImplicitArgumentConversions(S, static_cast<CallExpr*>(E), CC);

Instead of isa<> and then static_cast<> (which should be cast<>
really), I think it's more common to do:

if (CallExpr *Call = dyn_cast<CallExpr>(E))
  CheckImplicitConversion(S, Call, CC);

> +// RUN: %clang_cc1 -x c++ -verify -fsyntax-only -Wimplicit-conversion-floating-point-to-bool %s

I don't think '-x c++' is necessary; Clang will pick it up based on
the file extension.

> +float  foof (float x);
> +double food (double x);
> +void   foo  (bool b, float f);

Having a space between the function name and the '(' looks a little
unusual. I don't think there's any reason to line up the functions
this way.

> +  b = foof(c < 1); // expected-warning {{implicit conversion turns floating-point number into bool: 'float' to 'bool'}}

Hmm, so I guess this won't warn in C, where the type of 'c < 1' is int
rather than bool? Might not be a big deal, but it seems a little
unfortunate.