Chris Lattner wrote: > On Oct 31, 2008, at 7:43 AM, Sebastian Redl wrote: >> URL: http://llvm.org/viewvc/llvm-project?rev=58509&view=rev >> Log: >> Implement semantic checking of static_cast and dynamic_cast. > > Very nice Sebastian, thank you for your contribution! > > Some thoughts below. I'm leaving the "deep" issues to Doug, but I'll > nit pick some details :) Thanks for the review. I'll address your comments below and update the code as soon as I get my next piece of work out of the way. > > >> +++ cfe/trunk/include/clang/Basic/DiagnosticKinds.def Fri Oct 31 >> 09:43:28 2008 >> @@ -1096,6 +1096,8 @@ >> "an extension") >> DIAG(err_bad_reinterpret_cast_small_int, ERROR, >> "cast from pointer to smaller type '%0' loses information") >> +DIAG(err_bad_dynamic_cast_operand, ERROR, >> + "'%0' is %1") > > This is really dangerous. I note that you use this like this: > <snip> > etc. The problem with this is that we'd like to keep diagnostics > localizable. In practice, this means that the only thing that can be > substituted into a "%" variable safely are 1) standard language > keywords 2) things that are reprinted from the user's source code > (e.g. types). > > Substituting in random text won't work, because we lose the ability to > just localize the diagnostic catalog. Please split these into > separate diagnostics for each case. Will do. I expected this objection to be raised, but I wasn't entirely sure about best practice, so I went with the way that was less work ;-) > > >> ============================================================================== >> >> >> --- cfe/trunk/lib/AST/Expr.cpp (original) >> +++ cfe/trunk/lib/AST/Expr.cpp Fri Oct 31 09:43:28 2008 >> @@ -1072,15 +1072,17 @@ >> /// integer constant expression with the value zero, or if this is >> one that is >> /// cast to void*. >> bool Expr::isNullPointerConstant(ASTContext &Ctx) const { >> - // Strip off a cast to void*, if it exists. >> + // Strip off a cast to void*, if it exists. Except in C++. >> if (const ExplicitCastExpr *CE = dyn_cast<ExplicitCastExpr>(this)) { >> + if(!Ctx.getLangOptions().CPlusPlus) { > > It's a minor detail, but please add a space after the if. The > prevailing heuristic is that we put a space between a keyword and open > paren, but not a space between an identifier and open paren (as in > function calls). I know. It's just that my habit is to have no space there, and I have to go over the code afterwards to find all these places. Sometimes I overlook one. > >> +++ cfe/trunk/lib/Sema/Sema.h Fri Oct 31 09:43:28 2008 >> @@ -707,13 +707,22 @@ >> SourceLocation LParenLoc, >> ExprTy *E, >> SourceLocation RParenLoc); >> >> +private: >> // Helpers for ActOnCXXCasts >> - bool CastsAwayConstness(QualType SrcType, QualType DestType); >> void CheckConstCast(SourceLocation OpLoc, Expr *&SrcExpr, QualType >> DestType); >> void CheckReinterpretCast(SourceLocation OpLoc, Expr *&SrcExpr, >> QualType DestType); >> void CheckStaticCast(SourceLocation OpLoc, Expr *&SrcExpr, QualType >> DestType); >> + void CheckDynamicCast(SourceLocation OpLoc, Expr *&SrcExpr, >> + QualType DestType, const SourceRange >> &DestRange); >> + bool CastsAwayConstness(QualType SrcType, QualType DestType); >> + bool IsStaticReferenceDowncast(Expr *SrcExpr, QualType DestType); >> + bool IsStaticPointerDowncast(QualType SrcType, QualType DestType); >> + bool IsStaticDowncast(QualType SrcType, QualType DestType); >> + ImplicitConversionSequence TryDirectInitialization(Expr *SrcExpr, >> + QualType >> DestType); > > Instead of these being methods on Sema, would it be reasonable to make > them static functions that take Sema as an argument? The benefit of > doing this is to reduce the number of methods on Sema. If it is > outrageously ugly to do this, please don't, but as a general principle > it is good to prefer static functions where possible. I believe that should be no problem at all. I'll have to check the functions individually. > >> DestType = Context.getCanonicalType(DestType); > > Does this code really need to strip down to canonical types? Your > code seems very clean and uses predicates like ->getAsFoo and > ->isFoo() consistently. If it is safe, it would be nice to just use > these predicates consistently and avoid canonicalizing them. The > major benefit of this is that it means you don't need an "orig" type > as well as the canonical one. > > If you decide that you need to canonicalize, I'd prefer the code to > keep the original type named "DestType" (for example) and name the > canonical type "CanDestType" or "DestTypeC" or something like that, to > make it obvious to the reader that the type has been canonicalized. OK, I'll check that as well. > > >> + // C++ 5.2.9p5, reference downcast. >> + // See the function for details. >> + if (IsStaticReferenceDowncast(SrcExpr, DestType)) { >> + return; >> + } >> + >> + // C++ 5.2.9p2: An expression e can be explicitly converted to a >> type T >> + // [...] if the declaration "T t(e);" is well-formed, [...]. >> + ImplicitConversionSequence ICS = TryDirectInitialization(SrcExpr, >> DestType); >> + if (ICS.ConversionKind != >> ImplicitConversionSequence::BadConversion) { >> + if (ICS.ConversionKind == >> ImplicitConversionSequence::StandardConversion && >> + ICS.Standard.First != ICK_Identity) >> + { > > Sometimes you're putting open braces on their own line, sometimes on > the same line as the if (likewise for method bodies). I personally > don't care which, but consistency is good. Most of the code puts the > braces on the same line of the if/method, so it's probably best to go > with that. > >> + if (SrcExpr->isLvalue(Context) != Expr::LV_Valid) { >> + return false; >> + } > > Another minor issue: generally we avoid braces for really simple if > conditions like this. If you really prefer them, they are ok and I'm > fine with keeping them, but if they don't add value, I have a slight > preference to elide them. Actually, my system is complex, but consistent. I put the brace on the same line unless I had to break the condition across lines. I find that having the brace on the same line when the condition is multiple lines long blurs the distinction between the condition and the statement body. As for eliding braces, I never do that. I simply like being able to put another statement in there (usually debug output) without having to check if there is a brace first. I've got no problem with changing that stuff, though. > >> + DestType = Context.getCanonicalType(DestType); >> + const ReferenceType *DestReference = DestType->getAsReferenceType(); >> + if (!DestReference) { >> + return false; >> + } >> + QualType DestPointee = DestReference->getPointeeType(); >> + >> + QualType SrcType = Context.getCanonicalType(SrcExpr->getType()); >> + >> + return IsStaticDowncast(SrcType, DestPointee); > > Instead of requiring all the callers to pass in canonical types, it > would be cleaner (from an interface perspective) to make > IsStaticDowncast canonicalize the type if it really needs to. This > makes the caller simpler and makes the function more general. It's > even better to just not require canonical types of course ;-) I'll have to check if I require canonical types there. However, the function in question is really intended to only be called from two callers, which already use canonical types, and it seems a waste to canonicalize those types again. > >> +/// IsStaticDowncast - Common functionality of >> IsStaticReferenceDowncast and >> +/// IsStaticPointerDowncast. Tests whether a static downcast from >> SrcType to >> +/// DestType, both of which must be canonical, is possible and allowed. >> +bool >> +Sema::IsStaticDowncast(QualType SrcType, QualType DestType) >> +{ >> + assert(SrcType->isCanonical()); >> + assert(DestType->isCanonical()); >> + >> + if (!DestType->isRecordType()) { >> + return false; >> + } >> + >> + if (!SrcType->isRecordType()) { >> + return false; >> + } > > This is another very personal issue, but I'd generally prefer to merge > these into the same if, and use "||". There should also be a small > comment explaining why you reject non record types. I'll merge them. As for the comment, I think it's unnecessary. The function checks whether this is a cast in a class hierarchy - it should be completely obvious why these need to be records. > >> + // Assumptions to this point. >> + assert(DestPointer || DestReference); >> + assert(DestRecord || DestPointee->isVoidType()); >> + assert(SrcRecord); > > This is a self-evident/redundant comment :). It would be better to > enhance the assertions with a message explaining why these must hold, > e.g.: > > assert(SrcRecord && "Non-records were rejected above") > > or something like that. Short messages like this are very useful when > someone else is debugging a problem in your code, because it makes it > more obvious *why* you thought the condition should hold. OK. > >> bool Sema::IsIntegralPromotion(Expr *From, QualType FromType, >> QualType ToType) >> { >> const BuiltinType *To = ToType->getAsBuiltinType(); >> + if (!To) { >> + return false; >> + } > > This deserves a comment. Maybe something like "all integers are > builtin types"? OK.
As I said, it will be a few days before I commit fixes to these. I have some partial work in that area, and separating that stuff is so annoying in SVN. Sebastian _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
