[PATCH] unique_ptrify llvm::getLazyBitcodeModule's MemoryBuffer parameter

[David Blaikie]

On Wed, Aug 13, 2014 at 10:06 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> On 13 August 2014 12:55, David Blaikie <dblaikie at gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 9:03 AM, Rafael Espíndola
>> <rafael.espindola at gmail.com> wrote:
>>> I am somewhat OK with this approach. I think the one item I feel
>>> should be done differently is llvm::parseBitcodeFile. It can take just
>>> a stringref and build a fake membuf to pass down.  Maybe we should do
>>> that first (attached patch)?
>>
>> I'm curious - why pass by StringRef if a MemoryBuffer is needed after
>> anyway? If getLazyBitcodeModuleImpl doesn't need the MemoryBuffer's
>> name - why not pass by StringRef alone? (well, ownership, yeah... -
>> but it seems you called out this particular usage for
>> MemoryBuffer->StringRef->MemoryBuffer and I'm just confused as to why)
>
> Simplicity of the interface.
>
> The idea is that in the non-lazy case the interface can be as simple
> as "given a series of bytes, give back a Module".

Agreed, if it doesn't need a name.

Though the same argument applies to "getBitcodeTargetTriple" I assume
- which could be changed similarly. That was one of the comments I
made in the original review: "one possible option would be to create
new
shallow MemoryBuffers in these cases".

> It is an
> implementation detail that internally that is implemented on top of
> the lazy interface that has a more complicated ownership rules.

Well, not entirely an implementation detail. It's relevant to the user
that the Module doesn't just read from that StringRef on construction
- it's lazy, it keeps a reference to that memory and reads from it
arbitrarily later when the Module operations are used.

Not that that's a completely unique/new/novel API design - we take
references to things that are used for the lifetime of the object
being constructed all the time... but it's not nothing.

> This should also open up the possibility for in the future having the
> lazy interface never own the buffer. This would require convenience
> functions that take a filename to return a pair of
> Module,std::unique_ptr<MemoryBuffer>, but I think that is doable.
>
> There are other cases in LLVM where that is the case.

Which ones do you have in mind? I'd like to get a good sense of what's
done where/why, if there are existing idioms that we want to promote
to be the preferred solutions, etc.

>> Does getLazyBitcodeModuleImpl not need the MemoryBuffer's name?
>
> Doesn't look like it,

Curious - how can/did you tell?

> but if needed I think it should still be two StringRefs.

Two StringRefs even rather than your MemoryBufferRef? What do you see
as the distinction between the use of each of those, if any?

- David