[cfe-dev] [PATCH] Add APValue::swap

[Daniel Dunbar]

On Thu, Mar 8, 2012 at 10:37 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> Thanks Richard,
>
> I'm not really a C++ guy, maybe I wasn't expecting copy elision to do
> what I wanted. I'll try and look into your patch today and see why my
> numbers look like for it.

Just tried your patch, on my three standard tests I see 1-2% slowdown
from it. Maybe the code size bloat from inlining swap()? I didn't
investigate any more...

 - Daniel

>
>  - Daniel
>
> On Wed, Mar 7, 2012 at 9:17 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> The attached patch is the direction I was thinking of. I believe the changes
>> outside APValue are all redundant with this approach; this is a pure
>> optimization of the existing interface. The ExtractSubobject change isn't
>> necessary, but it avoids some potentially-expensive copying. Some care is
>> required to avoid it creating a cyclic value.
>>
>> I've not got any performance numbers for this change yet.
>>
>> On Wed, Mar 7, 2012 at 5:15 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>
>>> On Wed, Mar 7, 2012 at 3:05 PM, Daniel Dunbar <daniel at zuster.org> wrote:
>>>>
>>>> Here is a patch which adds APValue::swap and uses it.
>>>>
>>>> I'm not sure I like it, anyone else have an opinion for putting it in or
>>>> not?
>>>
>>>
>>> This was actually something I was intending to investigate soon. A few
>>> thoughts:
>>>
>>> I'd prefer to see operator= changed as follows, all the code which says
>>> 'foo = APValue(...);' left alone:
>>>
>>> APValue &operator=(APValue RHS) {
>>>   swap(RHS);
>>>   return *this;
>>> }
>>>
>>> Copy elision will do the rest. I'd then prefer to see copy_from killed,
>>> with the implementation moved to APValue's copy constructor (which should
>>> then be out-of-line).
>>>
>>> Do you have any performance numbers for this?
>>
>>