[llvm] r211595 - Pass a unique_ptr<MemoryBuffer> to the constructors in the Binary hierarchy.

[Sean Silva]

On Fri, Jul 4, 2014 at 3:39 PM, Rafael Espíndola <rafael.espindola at gmail.com
> wrote:

> > Hi Rafael,
> >
> > It doesn't make much sense to me that these objects own the buffer. After
> > all they only read from the buffer, and there's no reason why it
> shouldn't
> > be shared.
>
> In a way that is true, but then they should *never* delete the buffer.
> The old situation where they would sometimes own it was very error
> prone.
>
> > As an implementation detail, it's OK if they delete the buffer for now
> > (r211542 is fantastic) but I'm not at all keen on expressing that in the
> > interface because it ends up formalizing what's really a flaw in the
> > implementation.
>
> Well, we should have an interface that makes the ownership clear. If
> it possible to make them never own the buffer, that would be awesome.
> Once that is the case the constructors should probably be changed to
> even take a StringRef and a name instead of MemoryBuffer, but if they
> own what they point to then a unique_ptr is the correct interface
> IMHO.
>
> Would passing a pair of StringRefs (Data and Name) work for you?
>

That seems like the natural API.

-- Sean Silva


>
> > I have a changeset that makes these objects use the MemoryBuffer without
> > deleting it, but r211595 and r211546 get in the way of the approach by
> > making the interface require unique ownership.
> >
> > (I was expecting the underlying changes to land well before now which is
> why
> > I didn't speak up earlier, could you take a look at
> >
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140630/224492.html
> > ?)
>
> I consider reference counting to be the lest desirable option of all.
> It makes it incredibly hard to reason about the ownership, even more
> so than the old "owned" bool.
>
> Cheers,
> Rafael
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140705/10697287/attachment.html>