[cfe-dev] Preliminary reinterpret_cast Sema patch

[Sebastian Redl]

Doug Gregor wrote:
> 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:
>   
>> +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.
>   
No, I mean it exactly like it's there. var3->var4 does cast away 
constness (const& -> &). var2->var3 should be implicit, but that fails, 
so I made it a static_cast. var4->var5 tests the dropping of the 
reference, not casting away constness.
> 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);
>   
I suppose, for those cases where I don't use the variable.
>> +  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);
>   
These test array decay and that a function pointer pointer is not 
misidentified as a function pointer, not constness. I like my unit tests 
to test just one thing.
> Note that these expected-errors don't match the actual error messages
> we get, so -verify fails.
>   
Yeah, I changed the messages in the last reinterpret_cast update and 
forgot to update the test case.
> Please put a "// C++ casts" before all of the C++ cast-specific diagnostics.
>   
OK.
>> +/// CheckConstCast - Check that a const_cast<DestType>(SrcExpr) is
>>     
>
> The < should be a '<' here.
>   
Doxygen doesn't mind? I know that JavaDoc can get confused.
> 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.
>   
OK.
> 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.
>   
I like having the entire relevant standard parts summarized at the start 
of the function, but I agree that it results in quite a bit of info 
duplication, because I repeat some of it at the places I actually 
implement it.
>   
>> +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,
Usually, I do too, but I already had them in this scope because of the 
loop later, and I thought it ugly to have them again, so I just moved 
the declarations up here.
>> +    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)
>   
As I said, duplication.
>
>> +    if ((SrcTypeTmp = SrcType->getAsReferenceType()))
>> +      SrcType = SrcTypeTmp->getPointeeType();
>>     
>
> This shouldn't happen, because an expression never has reference type.
>   
Really? What type does "static_cast<int&>(i)" have? What about simply 
"i", where i is declared as "int &i = ...;"? Do they have type int, with 
lvalue possibility?
> Here's another case where citing the relevant part of p1 would help the reader.
>   
OK.
> Please note in the comment here that if DestType was a reference type
> originally, it is now a pointer type.
>   
OK.
>> +  }
>> +  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.
>   
Can't. The loop header also checks if the result of the assignment is 
null. I'd have to write something like:
while(SrcType != DestType) {
  const PointerType *SrcTypeTmp = SrcType->getAsPointerType();
  const PointerType *DestTypeTmp = DestType->getAsPointerType();
  if(!SrcTypeTmp || !DestTypeTmp) {
    break;
  }
  // ...
}
and I actually think my current code is nicer. (Matter of opinion, of 
course. I'll change it if you insist.)
>   
>> +  {
>> +    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.
>   
I don't see how. It costs us at most one loop iteration more. 
T1***const* is a different type from T1****. T1***const is a different 
type from T1*** (that's the wasted iteration). T1** is the same type as 
T1**.
The change makes sense, but I don't think it's as bad as you think.
> Also: is this going to allow us to const_cast between different
> address spaces? I'm not sure what the right answer is there.
>   
I don't even know what you mean by different address spaces, or how they 
are implemented. Are you talking about x86-16-type segmented memory or 
something even more extreme?
> 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.
>   
I wasn't aware of this guarantee - in fact, I was somehow under the 
impression that getCanonicalType only canonicalized the first level. 
Can't remember why I thought that, but I know that I very deliberately 
added these additional calls. I'll try it without them.
>   
>> +  // 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 can try it tomorrow.
>  I'm also inclined to diagnose this as an
> error, unless our reading proves faulty.
>   
Or at least emit a warning or something. I wonder if any real-world code 
actually relies on this quirk of the compilers.
> 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.
>   
Thank you for the review.

Sebastian