[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

[Chris Lattner]

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 :)


> +++ 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:

> +    Diag(OpLoc, diag::err_bad_dynamic_cast_operand,
> +      OrigDestType.getAsString(), "not a reference or pointer",  
> DestRange);


> +      Diag(OpLoc, diag::err_bad_dynamic_cast_operand,
> +        DestPointee.getUnqualifiedType().getAsString(),
> +        "incomplete", DestRange);


> +    Diag(OpLoc, diag::err_bad_dynamic_cast_operand,
> +      DestPointee.getUnqualifiedType().getAsString(),
> +      "not a class", DestRange);


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.


> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- 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).

> +++ 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.

>   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.


> +  // 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.

> +  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 ;-)

> +/// 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.

> +  // Comparing cv is cheaper, so do it first.
> +  if (!DestType.isAtLeastAsQualifiedAs(SrcType)) {
> +    return false;
> +  }

In contrast with the two above, keeping this one separated out seems  
to add value, because it gives you a nice place to put the comment.

> +  // 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.

> 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"?

Thanks again for the contribution, this is excellent work!

-Chris