[cfe-dev] static_cast and dynamic_cast semantic checks

[Sebastian Redl]

Doug Gregor wrote:
> On Wed, Oct 29, 2008 at 4:39 PM, Sebastian Redl
> <sebastian.redl at getdesigned.at> 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. :-)
>  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