[cfe-dev] static_cast and dynamic_cast semantic checks

[Doug Gregor]

On Wed, Oct 29, 2008 at 4:39 PM, Sebastian Redl
<sebastian.redl at getdesigned.at> wrote:
> Sebastian Redl wrote:
>>
>> Hi,
>>
>> Attached patch contains semantic checks for dynamic_cast and static_cast,
>> as far as the current class support allows either. This also subsumes the
>> small patch I asked permission for on Friday, which was a side effect of my
>> static_cast work.
>
> Another day, another update.

Thanks! I'll try not to keep stepping on your toes with these changes.

In Sema::CheckStaticCast:

+  // This option is unambiguous and simple, so put it here.
+  // C++ 5.2.9p4: Any expression can be explicitly converted to type "cv void".
+  if (DestType.getUnqualifiedType() == Context.VoidTy) {
+    return;
+  }

You should use "DestType->isVoidType()" here, to catch typedefs of void.

+  // Reverse integral promotion/conversion. All such conversions are themselves
+  // again integral promotions or conversions and are thus already handled.
+  // (Note: any data loss warnings should be suppressed.)
+  // The exception is the reverse of enum->integer, i.e. integer->enum (and
+  // enum->enum). See also C++ 5.2.9p7.
+  // The same goes for reverse floating point promotion/conversion and
+  // floating-integral conversions. Again, only floating->enum is relevant.
+  if (DestType->isEnumeralType()) {
+    if ((SrcType->isRealType() && !SrcType->isVectorType()) ||
+        SrcType->isEnumeralType())
+    {
+      return;
+    }
+  }

In the comment, it would help the reader if "already handled"
mentioned where/how these conversions were handled. I realized
(eventually) that it's a result of the implementation of paragraph 2,
because the integral promotions/conversions, floating point
promotions/conversions, and floating integral conversions are all
reversible.

isRealType threw me for a loop, since it's a C99 notion used in a C++
context. isArithmeticType matches the idea better (and the notion
exists in both C99 and C++). I think I'd like the inner "if" to look
like this:

    if (SrcType->isComplexType() || SrcType->isVectorType()) {
      // Fall though; no conversion from complex -> enum or vector -> enum.
    } else if (SrcType->isArithmeticType() || SrcType->isEnumeralType()) {
     return;
    }

Moving on...

+  // Reverse pointer upcast. C++ 4.10p3 specifies pointer upcast.
+  // C++ 5.2.9p8 additionally disallows a cast path through virtual
inheritance.
+  // (This is a practical limitation.)
+  if (IsStaticPointerDowncast(SrcType, DestType)) {
+    return;
+  }

What does the comment (This is a practical limitation) mean? (Do we need it?)

+  if (const PointerType *SrcPointer = SrcType->getAsPointerType()) {
+    QualType SrcPointee = SrcPointer->getPointeeType();
+    if (SrcPointee.getUnqualifiedType() == Context.VoidTy) {

The last "if" should use the condition SrcPointee->isVoidType().

+  // We tried everything. Everything! Nothing works! :-(
+  // FIXME: Error reporting could be a lot better. Should store the reason
+  // why every substep failed and, at the end, select the most specific and
+  // report that.
+  Diag(OpLoc, diag::err_bad_cxx_cast_generic, "static_cast",
+    OrigDestType.getAsString(), OrigSrcType.getAsString());

If you figure out a good way to do this, I'd be interested :)
Don't hold up your patch for it, of course. We can do that that later.

In Sema::IsStaticDowncast:

+  BasePaths::paths_iterator Path = Paths.begin(), Path2 = Path;
+  ++Path2;
+  if (Path2 != Paths.end()) {
+    // If there is more than one path, but there's no ambiguity, there's got
+    // to be a virtual inheritance in the paths.
+    return false;
+  }
+
+  for (BasePath::const_iterator PElem = Path->begin();
+       PElem != Path->end(); ++PElem)
+  {
+    if (PElem->Base->isVirtual()) {
+      return false;
+    }
+  }

This code is checking whether SrcType is a virtual base of DestType.
It definitely works as it is, but I think it could be both cleaner and
more efficient if we moved the "is one of the paths virtual?" check
into the BasePaths class. Then, Sema::IsDerivedFrom could just compute
this flag as it's looking for the base class. Doing this,
IsStaticDowncast would be able to set the BasePaths instance to avoid
record paths, which will be more efficient. One could also imagine
teaching Sema::IsDerivedFrom to abort earlier if it finds a path to a
virtual base, using the same logic added to track whether there is a
path through a virtual base.

In Sema::CheckDynamicCast:

+    // Reference to void should be impossible.
+    assert(DestPointer);

With this kind of thing, our scheme is to write the assert as:

  assert(DestPointer && "Reference to void is not possible");

That way, the message spit out by assert() when it fails tells us what
went wrong (roughly!).

+  // C++ 5.2.7p5
+  // Upcasts are resolved statically.
+  if (DestRecord && IsDerivedFrom(SrcPointee, DestPointee)) {
+    CheckDerivedToBaseConversion(SrcPointee, DestPointee, OpLoc,
SourceRange());
+    // Diagnostic already emitted on error.
+    return;
+  }

Shouldn't the last argument to CheckDerivedToBaseConversion be the
source range containing the full static_cast expression?

 bool Sema::IsIntegralPromotion(Expr *From, QualType FromType, QualType ToType)
 {
   const BuiltinType *To = ToType->getAsBuiltinType();
+  if (!To) {
+    return false;
+  }

   // An rvalue of type char, signed char, unsigned char, short int, or
   // unsigned short int can be converted to an rvalue of type int if
   // int can represent all the values of the source type; otherwise,
   // the source rvalue can be converted to an rvalue of type unsigned
   // int (C++ 4.5p1).
-  if (FromType->isPromotableIntegerType() &&
!FromType->isBooleanType() && To) {
+  if (FromType->isPromotableIntegerType() && !FromType->isBooleanType()) {

You only need the top change or the bottom change, not both; I suggest
keeping the top one.

@@ -552,7 +560,7 @@
         // We found the type that we can promote to. If this is the
         // type we wanted, we have a promotion. Otherwise, no
         // promotion.
-        return Context.getCanonicalType(FromType).getUnqualifiedType()
+        return Context.getCanonicalType(ToType).getUnqualifiedType()
           == Context.getCanonicalType(PromoteTypes[Idx]).getUnqualifiedType();
       }
     }

Argh! Thanks.

@@ -590,8 +600,9 @@

   // An rvalue of type bool can be converted to an rvalue of type int,
   // with false becoming zero and true becoming one (C++ 4.5p4).
-  if (FromType->isBooleanType() && To && To->getKind() == BuiltinType::Int)
+  if (FromType->isBooleanType() && To->getKind() == BuiltinType::Int) {
     return true;
+  }

   return false;
 }


We don't need this change, since the !To check is above.

 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)) {
-    // Check that it is a cast to void*.
-    if (const PointerType *PT = CE->getType()->getAsPointerType()) {
-      QualType Pointee = PT->getPointeeType();
-      if (Pointee.getCVRQualifiers() == 0 &&
-          Pointee->isVoidType() &&                                 // to void*
-          CE->getSubExpr()->getType()->isIntegerType())            // from int.
-        return CE->getSubExpr()->isNullPointerConstant(Ctx);
+    if(!Ctx.getLangOptions().CPlusPlus) {
+      // Check that it is a cast to void*.
+      if (const PointerType *PT = CE->getType()->getAsPointerType()) {
+        QualType Pointee = PT->getPointeeType();
+        if (Pointee.getCVRQualifiers() == 0 &&
+            Pointee->isVoidType() &&                              // to void*
+            CE->getSubExpr()->getType()->isIntegerType())         // from int.
+          return CE->getSubExpr()->isNullPointerConstant(Ctx);
+      }
     }
   } else if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(this)) {
     // Ignore the ImplicitCastExpr type entirely.

Good catch

I'd like to see if the Sema::IsStaticDowncast/BasePaths can be
optimized and cleaned up. Everything else is a minor comment, and
things are in very good shape. Thanks!

  - Doug