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. Note that only BuiltinType has the isFloatingPoint() method.
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.
warn-implicit-conversion-floating-point-to-bool.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
