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

[Chris Lattner]

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


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

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


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

-Chris