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 <[email protected]> wrote: > On Thu, Aug 16, 2012 at 4:23 PM, Andreas Eckleder <[email protected]> > 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
