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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
