[cfe-dev] Preliminary reinterpret_cast Sema patch

[Doug Gregor]

On Tue, Oct 21, 2008 at 9:44 AM, Sebastian Redl
<sebastian.redl at getdesigned.at> wrote:
>> Hrm, yeah. I guess what I don't like here is the use of
>> PointerLikeType (which, in truth, I don't really like anyway) as a
>> ReferenceType in some places and as a PointerType in other places.
>>
>
> OK, I split that into ReferenceType for the early checks and PointerType for
> the loop.

Thank you.

> Updated patch is attached. The reinterpret_cast tests have been made
> verifiable but are not yet in the tree.

Okay, thanks for the updated patch. Comments inline.

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

> +/// CheckConstCast - Check that a const_cast\<DestType\>(SrcExpr) is valid.
> +/// Refer to C++ 5.2.11 for details. const_cast is typically used in code
> +/// like this:
> +/// const char *str = "literal";
> +/// legacy_function(const_cast\<char*\>(str));
> +void
> +Sema::CheckConstCast(SourceLocation OpLoc, Expr *&SrcExpr, QualType
> DestType)

This routine looks good, thanks.

> +  // Doug Gregor said to disallow this until users complain.

I'll gladly take the blame :)

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

> +    // C++ 5.2.10p10: [...] a reference cast reinterpret_cast<T&>(x) has
> the
> +    //   same effect as the conversion *reinterpret_cast<T*>(&x) with the
> +    //   built-in & and * operators.
> +    // This code does this transformation for the checked types.
> +    DestType = Context.getPointerType(DestTypeTmp->getPointeeType());
> +    if (const ReferenceType *SrcTypeTmp = SrcType->getAsReferenceType()) {
> +      // FIXME: This shouldn't actually be possible, but right now it is.
> +      SrcType = SrcTypeTmp->getPointeeType();
> +    }
> +    SrcType = Context.getPointerType(SrcType);
> +  } else {
> +    // C++ 5.2.10p1: [...] the lvalue-to-rvalue, array-to-pointer, and
> +    //   function-to-pointer standard conversions are performed on the
> +    //   expression v.
> +    DefaultFunctionArrayConversion(SrcExpr);
> +    SrcType = SrcExpr->getType();
> +  }
> +
> +  // Canonicalize source for comparison.
> +  SrcType = Context.getCanonicalType(SrcType);
> +
> +  bool destIsPtr = DestType->isPointerType();
> +  bool srcIsPtr = SrcType->isPointerType();
> +  if (!destIsPtr && !srcIsPtr) {
> +    // Except for std::nullptr_t->integer, which is not supported yet, and
> +    // lvalue->reference, which is handled above, at least one of the two
> +    // arguments must be a pointer.
> +    Diag(OpLoc, diag::err_bad_cxx_cast_generic, "reinterpret_cast",
> +      OrigDestType.getAsString(), OrigSrcType.getAsString());
> +    return;
> +  }
> +
> +  if (SrcType == DestType) {
> +    // C++ 5.2.10p2 has a note that mentions that, subject to all other
> +    // restrictions, a cast to the same type is allowed. The intent is not
> +    // entirely clear here, since all other paragraphs explicitly forbid
> casts
> +    // to the same type. However, the behavior of compilers is pretty
> consistent
> +    // on this point: allow same-type conversion if the involved are
> pointers,
> +    // disallow otherwise.
> +    return;
> +  }
> +
> +  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).

> +    assert(srcIsPtr);
> +    // C++ 5.2.10p4: A pointer can be explicitly converted to any integral
> +    //   type large enough to hold it.
> +    if (Context.getTypeSize(SrcType) > Context.getTypeSize(DestType)) {
> +      Diag(OpLoc, diag::err_bad_reinterpret_cast_small_int,
> +        OrigDestType.getAsString());
> +    }
> +    return;
> +  }
> +
> +  if (SrcType->isIntegralType() || SrcType->isEnumeralType()) {

Despite the fact that SrcType->isIntegralType() also includes
SrcType->isEnumeralType(), please leave the || in here so we don't
forget later on that we also want to check enumeration types :)

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

> +  {
> +    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. 4.4p4, which describes the implicit
qualification conversion, doesn't require later levels of have exact
qualifiers. For this implicit qualification conversion, if there was a
"const" at an earlier level, there needs to be a const at later
levels, and you can only add (not remove) qualifiers;
Sema::IsQualificationConversion encodes this implicit conversion.

One easy way to check whether a conversion casts away constness would
be to actually construct the types described in 5.2.11p8 and use
IsQualificationConversion... when that is false, the reinterpret_cast
is casting away constness.

> +      Diag(OpLoc, diag::err_bad_cxx_cast_const_away, "reinterpret_cast",
> +        OrigDestType.getAsString(), OrigSrcType.getAsString());
> +      return;
> +    }
> +  }
> +
> +  if (SrcType == DestType) {
> +    // See above for the similar check.
> +    return;
> +  }
> +
> +  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.

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

> +    if (DestType->isFunctionType()) {
> +      // C++0x 5.2.10p8: Converting a pointer to a function into a pointer
> to
> +      //   an object type or vice versa is conditionally-supported.
> +      // Compilers support it in C++03 too, though, because it's necessary
> for
> +      // casting the return value of dlsym() and GetProcAddress().
> +      if (!getLangOptions().CPlusPlus0x) {
> +        Diag(OpLoc, diag::ext_reinterpret_cast_fn_obj);
> +      }

This makes me wonder if we should have some kind of option for
conditionally-supported behavior. /me files that in the back of his
mind

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.

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.

  - Doug