On Fri, Aug 24, 2012 at 6:00 PM, Hans Wennborg <[email protected]> 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 <[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
