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

[Bill Wendling]

On Apr 14, 2008, at 11:51 PM, Chris Lattner wrote:
>
> On Apr 6, 2008, at 3:48 AM, Bill Wendling wrote:
>
>> Attached is my latest patch for the C++ casting checks. It  
>> incorporates all of the feedback from Chris.
>
>
> Hi Bill, sorry for the delay,
>
> This is getting better, but still has canonical type problems:
>
> +static QualType BuildSubType(QualType Ty, unsigned Depth,  
> ASTContext &Ctx) {
> ...
>  assert(isa<PointerType>(Ty) && "This should be a pointer type!");
> +
> +  QualType Base = Ty->getAsPointerType()->getPointeeType();
>
>
> The assert should be Ty->isPointerType(), which is true even if 'Ty'  
> is a typedef for a pointer.  Your regtest should include some  
> typedefs ;-)
>
Typedefs are for the weak! ;-)

>
> +  if (Base.isConstQualified())    QT.addConst();
> +  if (Base.isVolatileQualified()) QT.addVolatile();
> +  if (Base.isRestrictQualified()) QT.addRestrict();
>
> You can just use:  QualType(QT.getTypePtr(), Base.getCVRQualifiers())
>
>
> +bool Sema::CastsAwayConstness(QualType CastFrom, QualType CastTo) {
> ..
> +  if (CastTo->isPointerType() && CastFrom->isPointerType()) {
>
> +    QualType From = BuildSubType(CastFrom, K, Context);
> +    QualType To = BuildSubType(CastTo, K, Context);
> +
> +    return CastsAwayConstnessFromPointers(From, To);
>
>
>
> Since 'BuildSubType' is O(n) in the pointer depth of its argument,  
> this means that 'CastsAwayConstness' is O(n^2).  Surely there has to  
> be a better way to evaluate this relation than with 'BuildSubType'?   
> Also, the name 'BuildSubType' is too generic to be useful.
>
There might be a more elegant way to check this, but the algorithm  
isn't O(n^2) just O(n) because it calls CastsAwayConstnessFromPointers  
instead of recursively calling CastsAwayConstness.

> ...
> +    if (!isa<PointerType>(To)) break;
> ...
>
> Should be To->isPointerType()
>
>
>
> +  if (CastTyBase.getTypePtr() == ExprTyBase.getTypePtr())
> +    return true;
>
> This isn't safe, one might be a typedef for the other.  Also, you  
> should have comments in your methods explaining why you are doing  
> what you are doing.
>
At this point, one of them won't be a pointer type. Essentially, what  
I want this method to do is go through all of the layers of both  
pointer types until it gets to the base type. It then checks to see if  
the base types are equal (the code you quoted above). I take it from  
your comment that this isn't sufficient to do that. How do I check  
past the typedefs when the types may not be pointers at all? Should I  
use "getDesugaredType()"?

> +  if (To == From) // {C++ [expr.const.cast] p2} Casting to same type.
> +    return To;
>
> Not safe with typedefs.
>
>
> Action::ExprResult
> Sema::ActOnCXXCasts(SourceLocation OpLoc, tok::TokenKind Kind,
> ...
> +  case tok::kw_const_cast:
> +    Op = CXXCastExpr::ConstCast;
> +    ResultTy = CheckConstCastOperator(OpLoc,  
> QualType::getFromOpaquePtr(Ty),
> +                                      Exp);
> ...
> +
> +  if (ResultTy.isNull()) {
> +    delete Exp;
> +    return true;
> +  }
> +
> +  return new CXXCastExpr(Op, ResultTy, Exp, OpLoc);
> }
>
> To simplify the code, I'd suggest 'new'ing the CastExpr in each case  
> arm, assigning into an OwningPointer.  The ActOnCall implementation  
> does this to good effect.
>
Okay. I'll get another submission to you soon.

-bw