static_cast and dynamic_cast semantic checks

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.

Sebastian

cast.patch (32.3 KB)

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.

Aaaaand ... here's the updated version, merged with Doug's big patch.

cast.patch (32.5 KB)

Sebastian Redl 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.

Aaaaand ... here's the updated version, merged with Doug's big patch.

And another one, merged with the reference patch and with the reference hack unwraps removed.

cast.patch (32 KB)

Sebastian Redl wrote:

Sebastian Redl 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.

Aaaaand ... here's the updated version, merged with Doug's big patch.

And another one, merged with the reference patch and with the reference hack unwraps removed.

Another update. This adds a rudimentary implementation of 8.5.3 (reference binding) to TryCopyInitialization. This allows references to bind to less cv-qualified versions of the same type as well as to subclasses of the reference type, which is a considerable improvement over the current situation. Subclass checking is not as stringent as it should be.
This isn't in the actual initialization code yet, but it works for static_cast and should work for argument passing.

Sebastian

cast.patch (33.9 KB)

Hah! I've also been implementing 8.5.3... I'll take a look.

  - Doug

And as expected, your implementation is a lot better structured. Mine was a
quick hack because I got an error that was annoying me.
I'll update my patch and send a new version today.

Sebastian

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.

Sebastian

cast.patch (31.5 KB)

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! :frowning:
+ // 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 :slight_smile:
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;
+ }

Doug Gregor wrote:

  

Another day, another update.
    
Thanks! I'll try not to keep stepping on your toes with these changes.
  

Don't worry about it. I don't see how you can avoid it anyway. And pretty much every change you do ultimately improves my code as well.

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.
  

OK.

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

OK.

isRealType threw me for a loop, since it's a C99 notion used in a C++
context.

I basically went hunting among the various functions of Type to find one which closely matched the simple concept I wanted - but those pesky complex and vector types got in the way. :slight_smile:

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

Looks good.

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

It was a thought I had and which I put in a comment in the flow. We don't need it.
As for what it means, it occurred to me at the time that a static cast through a virtual base was technically impossible, since you don't know the offset.

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

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

OK.

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.
  

Sounds great. Then I can also remember the virtual base and print it in the error message, as GCC does.

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!).
  

OK. Strange, I usually do this anyway, but sometimes I don't, and I never can remember why.

+ // 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?
  

Could be. On the other hand, I never mark the entire cast expression on conversion errors that involve both source and destination, so it would be a bit inconsistent.

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.
  

You seem to have this backward. There's a check for To not being null in your existing code. Since my newly introduced check assures this, I removed yours.

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

As above.

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!
  

OK, new patch to come soon. Thanks for the review.

Sebastian

Doug Gregor wrote:

+ // 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?

Could be. On the other hand, I never mark the entire cast expression on
conversion errors that involve both source and destination, so it would be a
bit inconsistent.

Hmmm... actually, I think I'd prefer if errors that involve both
source and destination always marked the whole cast expression. It
doesn't matter much in a terminal, but in an IDE it would be really
nice for the whole erroneous express to be marked.

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.

You seem to have this backward. There's a check for To not being null in
your existing code. Since my newly introduced check assures this, I removed
yours.

Yep, you're right. Thanks!

  - Doug

Doug Gregor wrote:

Hmmm... actually, I think I'd prefer if errors that involve both
source and destination always marked the whole cast expression. It
doesn't matter much in a terminal, but in an IDE it would be really
nice for the whole erroneous express to be marked.
  

OK, I'll do that for all the cast errors once I get my current work in.

Here's the updated version of the patch, now with virtual detection via BasePaths. Note that I use CXXRecordType in a few places, so if you commit your work there, this patch will be invalidated.

Sebastian

cast.patch (37.2 KB)

Doug Gregor wrote:

Hmmm... actually, I think I'd prefer if errors that involve both
source and destination always marked the whole cast expression. It
doesn't matter much in a terminal, but in an IDE it would be really
nice for the whole erroneous express to be marked.

OK, I'll do that for all the cast errors once I get my current work in.

Okay. You can go ahead and commit that directly; no need to post it here first.

Here's the updated version of the patch, now with virtual detection via
BasePaths. Note that I use CXXRecordType in a few places, so if you commit
your work there, this patch will be invalidated.

This looks good. Go ahead and commit at your leisure. Thanks!

  - Doug