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

[Andreas Eckleder]

Hi Hans,

Thanks again for the quick and helpful review! Please find attached a
patch that addresses most of the things you pointed out, except for
the following:
My concern with "return TypeA->isSpecificBuiltinType(SrcTy) &&
TypeB->isSpecificBuiltinType(DstTy);" is that there is float and
double as different floating point types, so I have to use
isFloatingPoint() on what I expect to be the floating point builtin
type. Note that only BuiltinType has the isFloatingPoint() method.

I've tried to come up with something more elegant than this, but I
decided not to check for float and double separately, simply because
the isFloatingPoint() method seems to be the canonical way to check
for a floating point type (as used in many other places within
SemaChecking.cpp).

Best Regards,



On Thu, Aug 23, 2012 at 6:04 PM, Hans Wennborg <hans at chromium.org> wrote:
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: warn-implicit-conversion-floating-point-to-bool.patch
Type: application/octet-stream
Size: 5961 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120824/177f49ac/attachment.obj>