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

[Nico Weber]

I tested this warning on chromium. It found 1 bug (
https://bugs.webkit.org/show_bug.cgi?id=94756 ) and 0 false positives.
+1 on turning it on by default after addressing Hans's comments.

Nico

On Thu, Aug 16, 2012 at 9:12 AM, Hans Wennborg <hans at chromium.org> wrote:
> 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
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits