[cfe-commits] [PATCH] Fix for reinterpret_cast (bug 11747)

[Eli Friedman]

On Sun, Jan 22, 2012 at 12:56 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Sun, Jan 22, 2012 at 9:48 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Sun, Jan 22, 2012 at 12:31 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> On Sun, Jan 22, 2012 at 8:58 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>>> On Sat, Jan 21, 2012 at 11:38 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>> On Thu, Jan 19, 2012 at 12:00 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>>>>> On Mon, Jan 16, 2012 at 8:38 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>>>> On Sun, Jan 15, 2012 at 9:05 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>>>>>>> On Sun, Jan 15, 2012 at 1:45 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>>>>>> This is a patch for fixing 11747, allowing reinterpret_cast to "cast"
>>>>>>>>> where the source and destination types are the same as per
>>>>>>>>> [expr.reinterpret.cast]p2.  The fix itself is fairly simple -- the
>>>>>>>>> code was already there, it just needed to be moved up slightly since
>>>>>>>>> the previous check was for source and dest being pointers.
>>>>>>>>
>>>>>>>> C++11 [expr.reinterpret.cast]p2 is as follows:
>>>>>>>>
>>>>>>>> The reinterpret_cast operator shall not cast away constness (5.2.11).
>>>>>>>> An expression of integral, enumeration, pointer, or pointer-to-member
>>>>>>>> type can be explicitly converted to its own type; such a cast yields
>>>>>>>> the value of its operand.
>>>>>>>
>>>>>>> Attached is a new patch to hopefully better address this, as well as
>>>>>>> some additional test cases.
>>>>>>
>>>>>> +    // C++ 5.2.10p2 has a note that mentions that, subject to all other
>>>>>> +    // restrictions, a cast to the same type is allowed so long as it does not
>>>>>> +    // cast away constness. The intent is not entirely clear here, since all
>>>>>> +    // other paragraphs explicitly forbid casts to the same type.
>>>>>> +    //
>>>>>> +    // The only allowed types are: integral, enumeration, pointer, or
>>>>>> +    // pointer-to-member types.
>>>>>>
>>>>>> This comment is really unclear... you should really explicitly state
>>>>>> that the C++98 version is unclear, and C++11 clarifies this case.
>>>>>
>>>>> I've clarified now, thanks for pointing out the confusion!
>>>>>
>>>>>> +    } else if (SrcType->isPointerType() ||
>>>>>> +      SrcType->isMemberPointerType()) {
>>>>>>
>>>>>> The original code allowed "DestType->isAnyPointerType() ||
>>>>>> DestType->isBlockPointerType()".  I don't see any reason to restrict
>>>>>> Objective-C or block pointers.
>>>>>
>>>>> Nor do I, I've added this in as well.
>>>>>
>>>>>> +      if (CastsAwayConstness(Self, SrcType, DestType, /*CheckCVR=*/!CStyle,
>>>>>> +                           /*CheckObjCLifetime=*/CStyle)) {
>>>>>>
>>>>>> How could a cast where the source and destination types are identical
>>>>>> cast away constness?
>>>>>
>>>>> From my testing, the equality test does not take cv qualifiers into
>>>>> account.  Is there another way I should be testing for equality that
>>>>> does?
>>>>
>>>> "==" should definitely be taking qualifiers into account.  Do you have
>>>> a testcase?
>>>
>>> It was in the patch -- but you can reproduce it yourself pretty easily:
>>>
>>> void foo() {
>>>  const int i = 0;
>>>  (void)reinterpret_cast< int >( i );
>>> }
>>>
>>> Put a breakpoint in SemaCast.cpp on or around line 1629 after applying
>>> the patch and you'll see that you break into the SrcType == DestType
>>> block.
>>
>> Isn't that just lvalue-to-rvalue conversion stripping off qualifiers?
>
> I think you're right -- as soon as I use pointers, the equality test
> fails as you predicted.  I've adjusted my patch accordingly, thanks
> for the thorough review!

+    // C++ 5.2.10p2 has a note that mentions that, subject to all other
+    // restrictions, a cast to the same type is allowed so long as it does not
+    // cast away constness. In C++98 The intent is not entirely clear here,
+    // since all other paragraphs explicitly forbid casts to the same type.
+    // In C++11 clarifies this case.

It looks like this comment somehow got messed up in editing.

Otherwise, looks good.

-Eli