[cfe-dev] Preliminary reinterpret_cast Sema patch

[Sebastian Redl]

Doug Gregor wrote:
> On Tue, Oct 21, 2008 at 9:44 AM, Sebastian Redl
> <sebastian.redl at getdesigned.at> wrote:
>   
>
>> +  case tok::kw_const_cast:
>> +    Op = CXXCastExpr::ConstCast;
>> +    CheckConstCast(OpLoc, Ex, DestType);
>> +    break;
>> +  case tok::kw_dynamic_cast:
>> +    Op = CXXCastExpr::DynamicCast;
>> +    break;
>> +  case tok::kw_reinterpret_cast:
>> +    Op = CXXCastExpr::ReinterpretCast;
>> +    CheckReinterpretCast(OpLoc, Ex, DestType);
>> +    break;
>> +  case tok::kw_static_cast:
>> +    Op = CXXCastExpr::StaticCast;
>> +    break;
>>     
>
> Hrm. It's not your problem, but I think we'll end up refactoring the
> cast hierarchy a bit. I really dislike having a CXXCastExpr node with
> an enum inside it telling us which case it is, because the casts are
> all *totally* different semantically. But, I'll make this change after
> your const_cast and reinterpret_cast patches are in the tree.
>   
Oh, absolutely! I left it the way it is now because it's irrelevant to 
the semantic checks I'm doing, but it was pretty clear the current 
scheme is insufficient.
>> +  DestType = Context.getCanonicalType(DestType);
>> +  QualType SrcType = SrcExpr->getType();
>> +  if (const ReferenceType *DestTypeTmp = DestType->getAsReferenceType()) {
>> +    if (SrcExpr->isLvalue(Context) != Expr::LV_Valid) {
>> +      // Cannot cast non-lvalue to reference type.
>> +      Diag(OpLoc, diag::err_bad_cxx_cast_rvalue,
>> +        "reinterpret_cast", OrigDestType.getAsString());
>> +      return;
>> +    }
>>     
>
> Hmm... in the case of an error when type-checking the casts, we should
> not actually build a CXXCastExpr node. Rather, Sema::ActOnCXXCasts
> should return "true" so that the parser knows that there was an error.
>
> I suggest that both CheckConstCast and CheckReinterpretCast return a
> bool that is true when there is an error and false otherwise (this is
> the scheme used elsewhere in Clang). Then, ActOnCXXCasts will never
> build an ill-formed node in the AST (which is a good thing).
>   
I had it that way originally, but Argiris told me to return the node 
regardless of the result of the check.
>> +  if (DestType->isIntegralType()) {
>>     
>
> At the moment, this needs to be DestType->isIntegralType() &&
> !DestType->isEnumeralType(), because Clang currently considers
> enumeration types to be integral types. (That's wrong for C++ but
> might be right for C; it needs more investigation).
>   
OK.
>> +  // Unwrap the pointers. Terminate early if the types are completely
>> equal.
>> +  while (SrcType != DestType &&
>> +    (SrcTypeTmp = SrcType->getAsPointerType()) &&
>> +    (DestTypeTmp = DestType->getAsPointerType()))
>>     
>
> When I added the qualification conversions, I put in the routine
> Sema::UnwrapSimilarPointerTypes that should help simplify the control
> flow of this loop. I suggest that you check it out. (It may also help
> CheckConstCast, but perhaps not as much, since that loop is a bit
> simpler).
>   
I was planning to. Of course, the patch is older than your commit.
>   
>> +  {
>> +    deepPtr = true;
>> +    SrcType = SrcTypeTmp->getPointeeType();
>> +    DestType = DestTypeTmp->getPointeeType();
>> +    // Qualifiers must be identical at later levels.
>> +    if (SrcType.getCVRQualifiers() != DestType.getCVRQualifiers()) {
>>     
>
> Hmmm, I don't think this is right.
Indeed. I realized that after reading your work on 
IsQualificationConversion.
I'll just reuse your function here.
>> +  if(deepPtr) {
>> +    // If we unwrapped more than one level, we're not dealing with function
>> or
>> +    // member pointers at the top level. This means we're done.
>> +    return;
>> +  }
>> +
>> +  // We have stripped away all indirections from at least one of the types.
>>     
>
> Ah, this is a bit subtle. The checks that follow are skipped if we
> have pointers-to-pointers. Personally, I think it would be cleaner if
> the "casting away constness" check didn't modify SrcType and DestType;
> then, pointers-to-pointers would just go through the tests below
> without complaining, since pointers are object types.
>   
OK.
>> +  // As for members, we don't have them yet.
>> +
>> +  if (SrcType->isObjectType() || SrcType->isVoidType()) {
>> +    if (DestType->isObjectType() || DestType->isVoidType()) {
>> +      // C++ 5.2.10p7: A pointer to an object can be explicitly converted
>> to
>> +      //   a pointer to an object of different type.
>> +      // Void pointers are not specified, but supported by G++.
>> +      return;
>> +    }
>>     
>
> EDG supports void pointers, too, and it seems obvious that they should
> be allowed, but there is nothing in the standard or in the issues list
> to imply that conversion to/from void pointers is okay.
>   
So, do we support this or not?
> It would be nice to have, in addition to the cxx-reinterpret-cast.cpp
> test, another "-pedantic -std=c++98" version of the test that tests
> the complaint about reinterpret_cast'ing pointer pointers to functions
> and pointers to objects.
>   
OK.
> Thanks again for the patch! We're getting close... actually, the
> const_cast bits could probably go in with a tiny amount of tweaking.
> If you'd like to separate out those bits and submit/commit, please go
> ahead; I'd like to look over a second round of the reinterpret_cast
> stuff before it goes in, though.
>   
I'll just post another full version of the patch. With SVN, separating 
different parts is always a bit of a hassle.

Sebastian