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

[Hans Wennborg]

On Thu, Aug 16, 2012 at 4:23 PM, Andreas Eckleder <aeckleder at google.com> wrote:
> Hello,
>
> A while ago, I posted a patch introducing a warning for suspicious
> implicit conversions from floating point to bool. Since then, I've run
> quite an extensive analysis of true and false positives this warning
> produces when applied to Google's code base.
>
> While the initial warning produced a low 4 digits number of hits with
> only about 10%-20% of them being sufficiently "suspicious" to merit
> further investigation, the new patch limits the warning two two
> specific cases with a 100% true positive rate:

Someone else should take a look too, but here are some comments from
looking at the patch itself:

Overall, there seems to be a numer of lines with 80+ columns. Also
everwhere a binary operator is used, there should be spaces around it.

> +def warn_impcast_floating_point_to_bool : Warning<
> +    "implicit conversion turns floating-point number into bool: %0 to %1">,
> +    InGroup<ImplicitConversionFloatingPointToBool>, DefaultIgnore;

If you had 0% false positives with this, maybe we should default it to
be enabled?

> +static bool ImplicitBoolToFloatArgumentConversion(Sema &S, Expr *Ex) {
Maybe name this "IsImplicitBoolToFloatConversion"?

> +  if (!isa<ImplicitCastExpr>(Ex))
> +    return false;
> +  Expr *InnerE = Ex->IgnoreParenImpCasts();
> +  const Type *Target = S.Context.getCanonicalType(Ex->getType()).getTypePtr();
> +  const Type *Source = S.Context.getCanonicalType(InnerE->getType()).getTypePtr();
> +  const BuiltinType *TargetBT = dyn_cast<BuiltinType>(Target);
> +  if ((!Target->isDependentType())&&(Source!=Target)&&
> +      Source->isSpecificBuiltinType(BuiltinType::Bool)&&
> +      TargetBT&&(TargetBT->isFloatingPoint())) {
> +    return true;
> +  }
> +  return false;

This if-statement is a hard to parse. Maybe things could be simplified
by doing some early returns. Perhaps something like this would work:

if (Target == Source)
    return false; // Maybe this isn't needed actually?
if (Target->isDepenedentType())
    return false;

return Source->isSpecificBuiltinType(BuiltinType::Bool)
        && Target->isSpecificBuiltinType(BuiltinType::Float);

> +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)) {

> +      Expr *InnerE = CurrA->IgnoreParenImpCasts();
> +      const Type *Target = S.Context.getCanonicalType(CurrA->getType()).getTypePtr();
> +      const Type *Source = S.Context.getCanonicalType(InnerE->getType()).getTypePtr();
> +      const BuiltinType *SourceBT = dyn_cast<BuiltinType>(Source);
> +      if ((!Target->isDependentType())&&(Source!=Target)&&
> +          Target->isSpecificBuiltinType(BuiltinType::Bool)&&
> +	  SourceBT&&(SourceBT->isFloatingPoint())) {

This is very similar to what's done in
ImplicitBoolToFloatArgumentConversion, but the other way around. Could
that function be parameterized so it could be used for both cases?
Maybe something like:

IsImplicitArgumentConversion(Sema &S, Expr *Ex, BuiltinType FromType,
BuiltinType ToType)

> +        if (((i>0) && ImplicitBoolToFloatArgumentConversion(S,TheCall->getArg(i-1))) ||
> +            ((i<(NumArgs-1)) && ImplicitBoolToFloatArgumentConversion(S,TheCall->getArg(i+1)))) {
> +          // Warn on this floating-point to bool conversion
> +          DiagnoseImpCast(S, InnerE, CurrA->getType(), CC, diag::warn_impcast_floating_point_to_bool);
> +        }

The code in this if statement and for loop in general is too tight for
my taste. Maybe it helps if some of it could be split out into the
function as commented above. I would also split the two cases of i
being confused with i+1 and i-1 into two different if statements.


> +    // If the target is bool, warn if expr is a function or method call
> +    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);
> +      unsigned NumArgs = CEx->getNumArgs();
> +      if (NumArgs>0) {
> +	Expr *LastA = CEx->getArg(NumArgs-1);
> +  	Expr *InnerE = LastA->IgnoreParenImpCasts();
> +	if (isa<ImplicitCastExpr>(LastA) &&
> +	   (S.Context.getCanonicalType(InnerE->getType()).getTypePtr()==Target)) {
> +	  // Warn on this floating-point to bool conversion
> +          DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_floating_point_to_bool);

Indentation looks off here.

> +// RUN: %clang_cc1 -x c++ -verify -fsyntax-only -Wimplicit-conversion-floating-point-to-bool %s
> +
> +float foof(float x) {
> +  return x;
> +}
> +
> +double food(double x) {

I guess you don't actually need to define the functions foof and food
for the test to work, just declarations should be sufficient.

Thanks,
Hans