[patch] Pass a MemoryBufferRef when we can avoid taking ownership
dblaikie at gmail.com
Tue Aug 26 14:20:08 PDT 2014
On Tue, Aug 26, 2014 at 2:04 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:
> On 26 August 2014 14:30, David Blaikie <dblaikie at gmail.com> wrote:
> > This patch essentially supercedes one I sent out for review a few
> > weeks ago? (that had to play games with ownership/unique_ptrs around
> > these functions due to the split ownership, etc) and takes the stance
> > of "construct a new shallow MemoryBuffer rather than lying to 'share'
> > unique ownership"?
> More or less. From my point of view the stance is that the interface
> should have the type that more directly corresponds to the interface
> semantics, not its implementation. For example
> * parseAssembly: without this patch it takes a MemoryBuffer to pass to
> a SourceMgr, but the fact that uses a SourceMgr should be an
> implementation detail and not change the interface.
> * parseBitcodeFile: It takes a MemoryBuffer because it is implemented
> on top of getLazyBitcodeModuleImpl, but that is also an implementation
> detail. We could have a separate non-lazy code path in the reader for
> > Might be worth changing getLazyBitcodeModuleImpl to take a
> > unique_ptr<MemoryBuffer>&& before you commit this patch? (that way you
> > don't have to play games with get/release after the successful call)
> You mean "unique_ptr<MemoryBuffer>&", right?
Actually I did mean && - it's not uncommon for APIs to take by movement
then fail to move-from in their failure path. Granted in the C++ standard
library that failure is demonstrated by throwing an exception, whereas this
has a return result.
> It actually looks easier
> to do that after my patch. I attached a followup patch.
Sure - looks basically right, though the release in getLzyBitcodeModuleImpl
is a bit scary. See my patch in the old review for another way to approach
this - using an RAII cleanup and being able to take ownership of the buffer
back from the BitcodeReader on the failure paths. I'm not sure it's the
right answer, but might be worth tossing around some thoughts.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits