r206986 - Replacing a naked pointer with a unique_ptr. No functional changes intended.

[Delesley Hutchins]

True.  :-)

  -DeLesley

On Wed, Apr 23, 2014 at 11:08 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Wed, Apr 23, 2014 at 10:58 AM, Delesley Hutchins <delesley at google.com> wrote:
>> Yes, Prev needs to be a pointer.  And it's non-owning, so IMHO, a raw
>> pointer is just fine.
>>
>> CallCtx does not need to be dynamically allocated, so long as you put
>> in a move assignment operator.
>> The copy is probably marginally cheaper than the dynamic
>> (de)allocation, it's just more code to write.  :-)
>
> I don't think you even need move semantics here, since the
> construction happens at the initialization (if you had to call some
> function that was returning a pointer to initialize the unique_ptr
> with that'd probably mean changing the return type to be return by
> value and using a move ctor or move assignment to put it into the
> Optional)
>
> CallCtx.reset(new SExprBuilder::CallingContext(FD));
>
> should just be:
>
> CallCtx.emplace(FD);
>
> ideally - which might require adding an "emplace" function to Optional
> (maybe check boost::optional/std::optional proposal to see how that
> operation is named in the standard) with the usual perfect forwarding
> etc
>
>>   -DeLesley
>>
>>
>> On Wed, Apr 23, 2014 at 10:23 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Wed, Apr 23, 2014 at 10:18 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>> On Wed, Apr 23, 2014 at 1:08 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>> On Wed, Apr 23, 2014 at 7:26 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>>> Author: aaronballman
>>>>>> Date: Wed Apr 23 09:26:59 2014
>>>>>> New Revision: 206986
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=206986&view=rev
>>>>>> Log:
>>>>>> Replacing a naked pointer with a unique_ptr. No functional changes intended.
>>>>>
>>>>> Any particular reason this needs to be dynamically allocated (it
>>>>> doesn't appear to be polymorphic - but perhaps it will be in the
>>>>> future?)? Would Optional<CallingContext> suffice?
>>>>
>>>> I think it would be possible to do -- CallingContext::Prev would have
>>>> to change to be Optional<CallingContext> Prev,
>>>
>>> Well, that wouldn't work (then you'd have an infinitely recursive type
>>> - optional has inline storage, so if a T has an Optional<T> then T has
>>> to be infinitely large), but it doesn't need to - it can remain as a
>>> non-owning pointer. (we could add a utility to Optional<T> to retrieve
>>> a T*, null if the Optional has no current value, if it doesn't already
>>> have such an operation)
>>>
>>>>but I don't see the
>>>> extra copies being overly expensive. I'll let DeLesley make the call
>>>> since this is in perf-sensitive code.
>>>>
>>>> ~Aaron
>>
>>
>>
>> --
>> DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315