[cfe-commits] r58509 - in /cfe/trunk: include/clang/Basic/DiagnosticKinds.def lib/AST/Expr.cpp lib/Sema/Sema.h lib/Sema/SemaExprCXX.cpp lib/Sema/SemaInherit.cpp lib/Sema/SemaInherit.h lib/Sema/SemaOverload.cpp test/SemaCXX/condition.cpp test/SemaCXX/dynamic-cast.cpp test/SemaCXX/static-cast.cpp

[Sebastian Redl]

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