[cfe-dev] Preliminary reinterpret_cast Sema patch
Sebastian Redl
sebastian.redl at getdesigned.at
Thu Oct 23 04:59:18 PDT 2008
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
More information about the cfe-dev
mailing list