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

[Hans Wennborg]

On Fri, Aug 24, 2012 at 6:00 PM, Hans Wennborg <hans at chromium.org> wrote:
> 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.

Landed r162763.

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