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

[David Blaikie]

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