[cfe-dev] Preliminary reinterpret_cast Sema patch

[Doug Gregor]

Hi Sebastian,

On Tue, Oct 21, 2008 at 8:25 AM, Sebastian Redl
<sebastian.redl at getdesigned.at> wrote:
> Doug Gregor wrote:
>> 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.

Ah, okay. The initialization of "var3" probably isn't checking what we
want it to check, because the semantics of reference binding aren't
implemented. Still, we're definitely checking *something*, so let's
leave it as is.

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

Now that I'm unconfused about the uses of var2, var3, etc., this
suggestion of mine doesn't matter so much.

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

Okay, good.

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

Ah, right. The right escaping is '\<', according to
http://www.stack.nl/~dimitri/doxygen/commands.html#cmdat

and you'll also need to escape the > with '\>'.

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

I guess it depends on how we read the comments... personally, I want
to get a rough idea of what the function is doing when I read the
function-level comment, then when I'm browsing the code I want to see
the correspondence between the standard and the code.

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

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.

>>> +    if ((SrcTypeTmp = SrcType->getAsReferenceType()))
>>> +      SrcType = SrcTypeTmp->getPointeeType();
>>>
>>
>> This shouldn't happen, because an expression never has reference type.
>>
>
> Really?

Yeah, check out C++ 5p5, where it talks about the adjustments to the
type of an expression. This is on my short list to fix in Clang.

> What type does "static_cast<int&>(i)" have?

The type is "int", and it's an lvalue.

> What about simply "i",
> where i is declared as "int &i = ...;"? Do they have type int, with lvalue
> possibility?

Yes, exactly.

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

I don't 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.

You're right; it's not as bad as I thought.

>>
>> 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?

We can ignore them for now with a FIXME. Address spaces are a C
extension for embedded platforms, which are represented by ASQualType
in Clang.

I *think* that const_cast should not permit one to change the address
space, but my understanding of address spaces is too fuzzy to be sure.

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

If one compiler gets it wrong, there is code somewhere that depends on
that behavior. It's a corollary to Murphy's Law for compilers :)

Still, if the code is ill-formed we should diagnose it up until the
point where users complain too loudly to ignore.

  - Doug