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

[David Blaikie]

On Fri, Jul 4, 2014 at 7:35 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 05/07/2014 04:59, Rafael Espíndola wrote:
>>>
>>> Where? *All* current uses on ToT seem to mmap their own private
>>> MemoryBuffer
>>> and un-mmap it immediately afterwards.
>>
>> Where? Show an strace where an **ObjectFile** is reopened.
>
>
> Trivially: just name them multiple times in llvm-dwarfdump/lli. That's
> probably fine, but less so with the proposed LTO object file patch where
> there's will be real cost.
>
>
>>
>> With object files we don't have the complexities of a source manager.
>> There are two reasonable ownership models: It owns the buffer and it
>> don't own the buffer. Not owning it might be more convenient,, I am
>> looking into it. A reference count makes no sense for the uses we
>> have.
>
>
> Reference counting is tangential to the discussion.
>
> The caller can do what they want to provision the MemoryBuffer instances --
> that could be reference counting, malloc or who knows, even stack
> allocation. It's invisible to ObjectFile as long as it only passes around
> pointers.
>
> Refcounting was just my suggestion for the slicing problem -- not a
> requirement or demand in any way, and you already said it's not needed..
>
>
>
>>
>> If the complexities of a source manager require a reference count to
>> avoid duplicating read and stats, then it should hold a list of
>> std::shared_ptr, or a list of a RefCountedMemoryBuffer. Changing how
>> ObjectFile works because a *source* manager wants to refcount buffers
>> is not OK.
>
>
> My point is that it's not OK to delete the caller's MemoryBuffer, nor does
> it make sense to pass it around in a std::unique_ptr given that the buffer
> is clearly shareable and holds no state.
>
> If you like you can store a pointer, keep a reference, or refcount -- but
> two commits look wrong because they enforce unique ownership on a resource
> that clearly shouldn't be uniquely owned.

The answer is fairly simple: Rafael's change didn't change any
ownership semantics, it merely made them more explicit. IMHO, this is
always an acceptable/good change. If someone wants to /change/ the
ownership, that's welcome (if the change is good), but just making the
ownership clear/explicit in the API should be an acceptable change,
even if that ownership is in some way not good. It's better that it's
clear.

(as evidence that this change did not change the ownership, merely
made it explicit:

-Binary::Binary(unsigned int Type, MemoryBuffer *Source)
-    : TypeID(Type), Data(Source) {}
+Binary::Binary(unsigned int Type, std::unique_ptr<MemoryBuffer> Source)
+    : TypeID(Type), Data(std::move(Source)) {}

(the type of the "Data" member was, before and after this patch,
std::unique_ptr<MemoryBuffer>))

>
>
> Alp.
>
>
>
>
>>
>> Cheers,
>> Rafael
>
>
> --
> http://www.nuanti.com
> the browser experts
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits