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

[Hans Wennborg]

Thanks, this looks good to me now. Assuming the mailing list starts
working soon, I'll get this landed for you assuming no one else has
any objections.

 - Hans

On Fri, Aug 24, 2012 at 1:45 PM, Andreas Eckleder <aeckleder at google.com> wrote:
> 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.

Ah, yes I didn't think about that. Your current approach is fine, then.


> 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.