[cfe-dev] Preliminary reinterpret_cast Sema patch

[Doug Gregor]

Hi Sebastian,

Thanks for the update; comments follow.

On Sun, Oct 19, 2008 at 7:26 AM, Sebastian Redl
<sebastian.redl at getdesigned.at> wrote:
> Argiris Kirtzidis wrote:
>>
>> And you are right, for C++0x it should probably be allowed without
>> pedantic warnings.
>
> GCC is not yet updated to do this, by the way. Yay, we've got better C++0x
> support (in a very minor subpoint). :-)
>
> Sebastian
>
> Index: test/SemaCXX/const-cast.cpp
> ===================================================================
> --- test/SemaCXX/const-cast.cpp (revision 0)
> +++ test/SemaCXX/const-cast.cpp (revision 0)
> @@ -0,0 +1,47 @@
> +// RUN: clang -fsyntax-only -verify %s
> +
> +// See if aliasing can confuse this baby.
> +typedef char c;
> +typedef c *cp;
> +typedef cp *cpp;
> +typedef cpp *cppp;
> +typedef cppp &cpppr;
> +typedef const cppp &cpppcr;
> +typedef const char cc;
> +typedef cc *ccp;
> +typedef volatile ccp ccvp;
> +typedef ccvp *ccvpp;
> +typedef const volatile ccvpp ccvpcvp;
> +typedef ccvpcvp *ccvpcvpp;
> +typedef int iar[100];
> +typedef iar &iarr;
> +typedef int (*f)(int);
> +
> +char ***good_const_cast_test(ccvpcvpp var)
> +{
> +  char ***var2 = const_cast<cppp>(var);
> +  char ***const &var3 = static_cast<cpppcr>(var2); // Different bug.
> +  char ***&var4 = const_cast<cpppr>(var3);
> +  char *** var5 = const_cast<cppp>(var4);

Did you really mean to use var2, var3, and var4 as the expressions in
these const_casts? The tests aren't actually casting away any
constness here, so I suspect that you want to use "var" as the
argument to the const_casts throughout.

Also, why create variables like var3 and var4? You can typically
express any "this expression has no effect"-type warnings (if that was
your intent) by writing, e.g.,

  (void)const_cast<cpppr>(var3);

> +  const int ar[100] = {0};
> +  int (&rar)[100] = const_cast<iarr>(ar);
> +  int *pi = const_cast<int*>(ar); // Array decay?
> +  f fp = 0;
> +  f *fpp = const_cast<f*>(&fp);

There doesn't seem to be any "const" involved here. I suggest adding, e.g.,

  f const * fp2 = 0;
  f* fpp2 = const_cast<f*>(fp2);

> +short *bad_const_cast_test(char const *volatile *const volatile *var)
> +{
> +  char **var2 = const_cast<char**>(var); // expected-error {{invalid
> const_cast from 'char const *volatile *const volatile *' to incompatible
> type 'char **'}}
> +  short ***var3 = const_cast<short***>(var); // expected-error {{invalid
> const_cast from 'char const *volatile *const volatile *' to incompatible
> type 'short ***'}}

Note that these expected-errors don't match the actual error messages
we get, so -verify fails.

> +  int *(*rar)[100] = const_cast<int *(*)[100]>(&ar); // expected-error
> {{invalid const_cast from 'int const *(*)[100]' to incompatible type 'int
> *(*)[100]'}}

This expected-error doesn't match the actual error message.

>  DIAG(err_value_init_for_array_type, ERROR,
>      "array types cannot be value-initialized")
> +DIAG(err_bad_cxx_cast_generic, ERROR,
> +     "%0 from '%2' to '%1' is not allowed")
[...]

Please put a "// C++ casts" before all of the C++ cast-specific diagnostics.

> +/// CheckConstCast - Check that a const_cast<DestType>(SrcExpr) is

The < should be a '<' here.

> valid.
> +/// 5.2.11/3: Both types must be pointers (to objects, void, or data
> members,
> +///   see 5.2.11/5) with the same level of indirection. The final pointee
> type
> +///   must be the same. All cv ([EXT] and r?) types along the way are
> mutable
> +///   by the cast. The result is an rvalue.
> +/// 5.2.11/4: If the incoming expression denotes an lvalue, DestType can be
> a
> +///   reference to the underlying type. The result is an lvalue.

A couple comments about the comments... please put "C++" prior to the
section/paragraph you are citing and use the "p" instead of the "/",
e.g., "C++ 5.2.11p4", so that the specification reference script can
pick up the references.

Also, I suggest moving the C++ standard quotes down into the body of
the function, where the code actually implements that rule. The
comment for CheckConstCast itself should point to the section in the
standard where const_cast is defined, and give a small example of how
const_cast is used.

> +void
> +Sema::CheckConstCast(SourceLocation OpLoc, Expr *&SrcExpr, QualType
> DestType)
> +{
> +  QualType OrigDestType = DestType, OrigSrcType = SrcExpr->getType();
> +
> +  DestType = Context.getCanonicalType(DestType);
> +  QualType SrcType = SrcExpr->getType();
> +  const PointerLikeType *SrcTypeTmp, *DestTypeTmp;

Okay, I know I'm being pretty picky, but I'd really prefer if
SrcTypeTmp and DestTypeTmp were only declared locally within the
blocks where they are used, e.g., replacing

> +  if ((DestTypeTmp = DestType->getAsReferenceType())) {

with

  if (const ReferenceTypeType *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,
> +        "const_cast", OrigDestType.getAsString());
> +      return;
> +    }
> +
> +    // /4: "if a pointer to T can be [cast] to the type pointer to T2"

Here's a case where citing all of p4 (and using the full spec
reference, [C++ 5.2.10p4)

> +    DestType = Context.getPointerType(DestTypeTmp->getPointeeType());


> +    if ((SrcTypeTmp = SrcType->getAsReferenceType()))
> +      SrcType = SrcTypeTmp->getPointeeType();

This shouldn't happen, because an expression never has reference type.
However, Clang is currently does this wrong, so I expect we'll need
this code for now. Please put a FIXME here pointing out that this "if"
should never be true.

> +    SrcType = Context.getPointerType(SrcType);
> +  } else {
> +    // Decay the argument.
> +    DefaultFunctionArrayConversion(SrcExpr);
> +    SrcType = SrcExpr->getType();

Here's another case where citing the relevant part of p1 would help the reader.

> +  }
> +  if (!DestType->isPointerType()) {
> +    // Cannot cast to non-pointer, non-reference type.
> +    Diag(OpLoc, diag::err_bad_const_cast_dest, OrigDestType.getAsString());
> +    return;

Please note in the comment here that if DestType was a reference type
originally, it is now a pointer type.

> +  }
> +  if (DestType->isFunctionPointerType()) {
> +    // Cannot cast direct function pointers.
> +    Diag(OpLoc, diag::err_bad_const_cast_dest, OrigDestType.getAsString());
> +    return;
> +  }
> +  SrcType = Context.getCanonicalType(SrcType);
> +
> +  // Unwrap the pointers. Ignore qualifiers. Terminate early if the types
> are
> +  // completely equal.
> +  while (SrcType != DestType &&
> +    (SrcTypeTmp = SrcType->getAsPointerType()) &&
> +    (DestTypeTmp = DestType->getAsPointerType()))

Please put SrcTypeTmp and DestTypeTmp (and their initializations)
inside the body of the while loop.

> +  {
> +    SrcType = Context.getCanonicalType(SrcTypeTmp->getPointeeType());
> +    DestType = Context.getCanonicalType(DestTypeTmp->getPointeeType());
> +  }

I'm very surprised to see that you aren't calling getUnqualifiedType
inside this loop. Calling getUnqualifiedType on each of SrcType and
DestType each time through would allow us to short-circuit the
comparison if we're casting, e.g., T1 * * * const * * * to T1 * * * *
* *; right now, it'll go through the loop all the way down until it
compares the T1's.

Also: is this going to allow us to const_cast between different
address spaces? I'm not sure what the right answer is there.

Finally, once SrcType and DestType are both canonical types (which is
guaranteed before we even enter the loop), all of the types we get via
getPointeeType will already be canonical. So, you don't need to call
getCanonicalType in the loop.

> +  // If we end up with constant arrays of equal size, unwrap those too. A
> cast
> +  // from const int [N] to int (&)[N] is invalid by my reading of the
> +  // standard, but g++ accepts it even with -ansi -pedantic.
> +  // No more than one level, though, so don't embed this in the unwrap loop
> +  // above.

How interesting... EDG also accepts this const_cast, but the standard
says absolutely nothing about being able to const_cast away qualifiers
on the element types of arrays. I'm curious whether MSVC diagnoses
this as an error or not. I'm also inclined to diagnose this as an
error, unless our reading proves faulty.

Thanks again for this patch; it's in very good shape already. I'll
follow up with a review of the reinterpret_cast implementation later.

  - Doug