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

[Alp Toker]

On 05/07/2014 00:39, Rafael Espíndola 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.

The problem with unique_ptr is that now there's no way to develop the 
right interface we want for file management, short of maintaining very 
large patches out-of-tree.

I really think we need to move to a model where buffers are owned singly 
and centrally (by a smarter SourceMgr, that can avoid re-opening files 
across compilations or strategising to use OS resources efficiently).


>
> Would passing a pair of StringRefs (Data and Name) work for you?
>
>> 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.

I agree with that, but reference counting for a few months gives us the 
clean pass-by-pointer interfaces we want in the long run. I'm not at all 
a fan of reference counting, but it's a tool we can use to get where we 
want.

Making the switch now sets things up so we can introduce proper 
ownership with very little code change.

So this definitely isn't about reference counting vs. something else -- 
it's about taking the first step to achieve the design we ultimately 
want, but always missed out on. My estimate is a 4-6 month timeframe to 
get there.

Alp.



>
> Cheers,
> Rafael

-- 
http://www.nuanti.com
the browser experts