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

[Chris Lattner]

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.

+    if (!isa<PointerType>(To)) break;
+
+    To = cast<PointerType>(To)->getPointeeType();
+    From = cast<PointerType>(From)->getPointeeType();

Not typedef safe.

+  if (CastTo->isReferenceType() && CastFrom->isReferenceType()) {
+    QualType From = cast<ReferenceType>(CastFrom)->getReferenceeType();
+    QualType To = cast<ReferenceType>(CastTo)->getReferenceeType();

likewise.  Many other examples as well.


-  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