[cfe-dev] Preliminary reinterpret_cast Sema patch

[Sebastian Redl]

Doug Gregor wrote:
> Hi Sebastian,
>
> On Tue, Oct 21, 2008 at 8:25 AM, Sebastian Redl
> <sebastian.redl at getdesigned.at> wrote:
>   
>> 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.
>   
It uses the C rules. They're not set up to handle references. However, 
they don't complain if you get exactly the same type, so since the 
expression static_cast<int&> has a reference type, this works. Note that 
this will break if you fix the bug you mentioned below.
>   
>>> 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.
>   
Meh, I did it anyway. It makes the tests a bit more readable. I also 
added comments to a few.
>
>>>> +/// 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 '\>'.
>   
Done.
>   
>>> 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.
>   
Done.
> 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.
> 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.
>   
You mean p6, but I see it. OK, added FIXME.
>> 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.
>   
I moved the declarations of the two variables just before the loop, though.
>>> 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.
>   
OK, added FIXME.
>   
>>>  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.
>   
OK, #ifdefed the code out for now.


Updated patch is attached. The reinterpret_cast tests have been made 
verifiable but are not yet in the tree.
By the way, I discovered that it emits a disambiguation warning on this 
line:
int (&rar)[100] = const_cast<iarr>(ar);
This feels a bit wrong - how else would I define a reference to an array 
to avoid the warning?

Sebastian
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cxx_cast.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081021/68568c55/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cxx-reinterpret-cast.cpp
Type: text/x-c++src
Size: 2331 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081021/68568c55/attachment.cpp>