[cfe-dev] For Review: C++ Casting Patch

[Bill Wendling]

On Mar 30, 2008, at 11:43 PM, Chris Lattner wrote:
> On Mar 28, 2008, at 10:03 PM, Bill Wendling wrote:
>
>> Attached is my long-awaited patch to implement checking of C++  
>> casting operators. It's not 100% complete, but it's a start in the  
>> right direction. It checks that the reinterpret_cast, static_cast,  
>> and dynamic_cast operators don't "cast away const". The const_cast  
>> can, of course, do this, but only if the underlying base type is  
>> the same.
>>
>> So a const_cast can cast from 'const double *' to 'double *', but  
>> not from 'const double *' to 'float *'. Whereas a reinterpret_cast  
>> can cast from 'const double *' to 'const float *', but not from  
>> 'const double *' to 'float *'. Etc.
>>
>> Also, a reinterpret_cast cannot cast to a type that has smaller  
>> precision. So you can't cast from a 'void *' to 'char'.
>
> Some thoughts:
>
> +    for (QualType QT = CastFrom; QT->isPointerType(); + 
> +DepthCastFrom)
> +      QT = cast<PointerType>(QT)->getPointeeType();
>
> This isn't safe: just because something "isPointerType()" doesn't  
> mean you can use cast<> (it might be a typedef).  I'd suggest:
>
>  PointerType *PT;
>  for (QualType QT = CastFrom; PT = QT->getAsPointerType(); + 
> +DepthCastFrom)
>    QT = PT->getPointeeType();
>
> This occurs a couple of times.
>
This shows the bitrot of this patch. I haven't kept up with all of the  
modifications you guys have made to the typing system. :-)

I'll make the changes, and do a clean-up that I was pondering over the  
weekend and submit another patch.

Thanks!
-bw

> -  return new CXXCastExpr(Op, QualType::getFromOpaquePtr(Ty),  
> (Expr*)E, OpLoc);
> +
> +  if (ResultTy.isNull())
> +    return true;
>
> Don't leak the argument expression if an error occurs.  You should  
> 'delete Exp'.
>
>
>
> +  // TODO: Finish.
>
> Be explicit, what is missing?  Also, tag with FIXME, not TODO.
>
> +      Diag(OpLoc, diag::err_cannot_cast_ptr_to_smaller_integral,
> +           From.getAsString(), To.getAsString(), SourceRange(OpLoc));
>
> Why are you making a source range on OpLoc?  That seems really  
> strange.  I'd suggest passing the sourcerange of "E" instead.
>
>
> You pass tons of stuff into CheckReinterpretCastOperator and friends  
> that isn't used, please remove the dead arguments.  The unused stuff  
> should be passed into ActOnCXXCasts (because it could be implemented  
> by a client that cares) but not into helpers in sema.
>
>
> +    QualType QT =
> +      CheckConstCastOperator(OpLoc, LAngleBracketLoc, TypeLoc,
> +			     Context.getPointerType(CT),
> +			     RAngleBracketLoc, LParenLoc,
> +                             E, OpLoc);
>
> Tabs.
>
>
> +bool Sema::CastsNullPointer(const Expr *Exp) {
> +  return Exp->isNullPointerConstant(Context);
> +}
>
> Why not just call isNullPointerConstant directly?  Also, this only  
> has one call site.
>
>
>
> +  /// C99: 6.7.5p3: Used by ParseDeclarator/ParseField to make sure  
> we have a
> +  /// constant expression of type int with a value greater than  
> zero.  If the
> +  /// array has an incomplete type or a valid constant size, return  
> false,
> +  /// otherwise emit a diagnostic and return true.
> +  bool VerifyConstantArrayType(const ArrayType *ary, SourceLocation  
> loc);
>
> This is dead.
>
> -Chris
>
>
>
>
>