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

[David Blaikie]

Looks like Rafael is already working on this in some more incremental
and probably better ways, so I'll defer to his work.

See Rafael's thread "[patch] Pass a MemoryBufferRef when we can avoid
taking ownership" for more details.

Here's a half-written email I had on this thread for a while. Just
sending for posterity.

On Tue, Aug 12, 2014 at 4:49 PM, Sean Silva <chisophugis at gmail.com> wrote:
> Maybe this is worth another thread on LLVMDev,

Possibly.

> but I really dislike using unique_ptr when it complicates things.

I'm not quite as adamant about that as you are - my take on it is that
unique_ptr demonstrates the complexity. The sort of stuff this code
was/is doing is /very/ subtle and making it more explicit seems good
to me.

> I see unique_ptr as a lightweight way ensure things are correct by default with no need to fiddle around; it should bring an immediate sense of security. Needing to have comments about ownership in code using unique_ptr defeats the purpose IMO.

Again, not quite sure that's the case - this change makes the "tricky"
code that's actually creating duplicate ownership more obvious and
makes the code that's not tricky (the code that actually calls
getLazyBitcodeModule and does really pass ownership of the
MemoryBuffer) safer - and that's where I started with this change, way
up in... somewhere I forget now.

> I feel like this patch is really complicating things, trying to move towards unique_ptr just for the sake of using unique_ptr. There's no difference between a unique_ptr that needs to have careful comments about ownership around it vs. a raw pointer with careful comments about ownership, besides the possibility that the unique_ptr will possibly lure the reader into a false sense of security.
>
> An unused-result call to .release() is as bad as raw new/delete: it is a manual, error-prone intervention for controlling an object's lifetime.

To be honest, given the code we live in today (with lots of raw
new/delete that was there long before unique_ptr) I actually find
"release()" to be a bigger red flag than new/delete - and I /like/
that.

It makes it clear that there's something tricky going on in this code
- and that wasn't as obvious in the old code, I don't think (perhaps
others disagree).

>
> ================
> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3552-3554
> @@ -3551,1 +3551,5 @@
> +ErrorOr<Module *>
> +llvm::getLazyBitcodeModule(std::unique_ptr<MemoryBuffer> &&Buffer,
> +                           LLVMContext &Context) {
> +  return getLazyBitcodeModuleImpl(std::move(Buffer), Context, false);
>  }
> ----------------
> What's the point of this? All std::move does is cast to T&&. You already have a T&&.

A named value is no longer an rvalue reference - you have to std::move
it again. Yeah, it's a bit surprising at first - but give it a go
(without the std::move, even in a simpler example) & you get used to
it :/

>
> http://reviews.llvm.org/D4876
>
>