True. :-) -DeLesley
On Wed, Apr 23, 2014 at 11:08 AM, David Blaikie <[email protected]> wrote: > On Wed, Apr 23, 2014 at 10:58 AM, Delesley Hutchins <[email protected]> > 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 <[email protected]> wrote: >>> On Wed, Apr 23, 2014 at 10:18 AM, Aaron Ballman <[email protected]> >>> wrote: >>>> On Wed, Apr 23, 2014 at 1:08 PM, David Blaikie <[email protected]> wrote: >>>>> On Wed, Apr 23, 2014 at 7:26 AM, Aaron Ballman <[email protected]> >>>>> 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 | [email protected] | 505-206-0315 -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
