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