[llvm] r213557 - Correct the ownership passing semantics of object::createBinary and make them explicit in the type system.

[Rafael Espíndola]

I will at least try passing two StringRefs (name and data) to the
Binary constructors. The binary would hold a std::string with the
name, but the data would be assumed to outlive the Binary.

The only oddity would be creating a "fake" MemoryBuffer is the
IRObjectFile to pass to IR reading code.


On 28 July 2014 17:25, David Blaikie <dblaikie at gmail.com> wrote:
> Sure thing - I tried to provide the same contract for
> SymbolicFile::createSymbolicFile, but couldn't - createSymbolicFile
> has a different contract (that it leaves the unique_ptr unmodified on
> failure) that is relied upon by llvm-ar with that releaseBuffer stuff
> you added/we talked about a while back. (so if createSymbolicFile
> succeeds, we've passed ownership to the SymbolicFile object, which we
> then retrieve later with releaseBuffer)
>
> Might it be nicer to, in llvm-ar, create a new MemoryBuffer (just a
> lightweight one, without copying the underlying data, from
> MemoryBuffer::createMemBuffer) and then that could be passed,
> unconditionally, to createSymbolicFile, and there'd be no need to
> release the buffer later? Then it could have a matching/simple
> contract rather than a "this might take ownership" contract.
>
> Little more allocation, though. And I still don't know what the right
> long-term answer is to MemoryBuffer (whether it will end up owning or
> non-owning) or I'd make more of an effort to avoid leaning on its
> conditional ownership semantics so much, in favor of whatever the
> long-term direction is.
>
> - David
>
> On Mon, Jul 28, 2014 at 2:08 PM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>> Thanks!
>>
>> On 21 July 2014 12:26, David Blaikie <dblaikie at gmail.com> wrote:
>>> Author: dblaikie
>>> Date: Mon Jul 21 11:26:24 2014
>>> New Revision: 213557
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=213557&view=rev
>>> Log:
>>> Correct the ownership passing semantics of object::createBinary and make them explicit in the type system.
>>>
>>> createBinary documented that it destroyed the parameter in error cases,
>>> though by observation it does not. By passing the unique_ptr by value
>>> rather than lvalue reference, callers are now explicit about passing
>>> ownership and the function implements the documented contract. Remove
>>> the explicit documentation, since now the behavior cannot be anything
>>> other than what was documented, so it's redundant.
>>>
>>> Also drops a unique_ptr::release in llvm-nm that was always run on a
>>> null unique_ptr anyway.
>>>
>>> Modified:
>>>     llvm/trunk/include/llvm/Object/Binary.h
>>>     llvm/trunk/lib/Object/Archive.cpp
>>>     llvm/trunk/lib/Object/Binary.cpp
>>>     llvm/trunk/tools/llvm-nm/llvm-nm.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/Object/Binary.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Binary.h?rev=213557&r1=213556&r2=213557&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Object/Binary.h (original)
>>> +++ llvm/trunk/include/llvm/Object/Binary.h Mon Jul 21 11:26:24 2014
>>> @@ -125,10 +125,8 @@ public:
>>>
>>>  /// @brief Create a Binary from Source, autodetecting the file type.
>>>  ///
>>> -/// @param Source The data to create the Binary from. Ownership is transferred
>>> -///        to the Binary if successful. If an error is returned,
>>> -///        Source is destroyed by createBinary before returning.
>>> -ErrorOr<Binary *> createBinary(std::unique_ptr<MemoryBuffer> &Source,
>>> +/// @param Source The data to create the Binary from.
>>> +ErrorOr<Binary *> createBinary(std::unique_ptr<MemoryBuffer> Source,
>>>                                 LLVMContext *Context = nullptr);
>>>
>>>  ErrorOr<Binary *> createBinary(StringRef Path);
>>>
>>> Modified: llvm/trunk/lib/Object/Archive.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Archive.cpp?rev=213557&r1=213556&r2=213557&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Object/Archive.cpp (original)
>>> +++ llvm/trunk/lib/Object/Archive.cpp Mon Jul 21 11:26:24 2014
>>> @@ -181,7 +181,7 @@ Archive::Child::getAsBinary(LLVMContext
>>>    if (std::error_code EC = BuffOrErr.getError())
>>>      return EC;
>>>
>>> -  return createBinary(*BuffOrErr, Context);
>>> +  return createBinary(std::move(*BuffOrErr), Context);
>>>  }
>>>
>>>  ErrorOr<Archive *> Archive::create(std::unique_ptr<MemoryBuffer> Source) {
>>>
>>> Modified: llvm/trunk/lib/Object/Binary.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Binary.cpp?rev=213557&r1=213556&r2=213557&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Object/Binary.cpp (original)
>>> +++ llvm/trunk/lib/Object/Binary.cpp Mon Jul 21 11:26:24 2014
>>> @@ -38,7 +38,7 @@ StringRef Binary::getFileName() const {
>>>    return Data->getBufferIdentifier();
>>>  }
>>>
>>> -ErrorOr<Binary *> object::createBinary(std::unique_ptr<MemoryBuffer> &Buffer,
>>> +ErrorOr<Binary *> object::createBinary(std::unique_ptr<MemoryBuffer> Buffer,
>>>                                         LLVMContext *Context) {
>>>    sys::fs::file_magic Type = sys::fs::identify_magic(Buffer->getBuffer());
>>>
>>> @@ -79,5 +79,5 @@ ErrorOr<Binary *> object::createBinary(S
>>>        MemoryBuffer::getFileOrSTDIN(Path);
>>>    if (std::error_code EC = FileOrErr.getError())
>>>      return EC;
>>> -  return createBinary(FileOrErr.get());
>>> +  return createBinary(std::move(*FileOrErr));
>>>  }
>>>
>>> Modified: llvm/trunk/tools/llvm-nm/llvm-nm.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-nm/llvm-nm.cpp?rev=213557&r1=213556&r2=213557&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/tools/llvm-nm/llvm-nm.cpp (original)
>>> +++ llvm/trunk/tools/llvm-nm/llvm-nm.cpp Mon Jul 21 11:26:24 2014
>>> @@ -993,13 +993,12 @@ static void dumpSymbolNamesFromFile(std:
>>>        MemoryBuffer::getFileOrSTDIN(Filename);
>>>    if (error(BufferOrErr.getError(), Filename))
>>>      return;
>>> -  std::unique_ptr<MemoryBuffer> Buffer = std::move(BufferOrErr.get());
>>>
>>>    LLVMContext &Context = getGlobalContext();
>>> -  ErrorOr<Binary *> BinaryOrErr = createBinary(Buffer, &Context);
>>> +  ErrorOr<Binary *> BinaryOrErr =
>>> +      createBinary(std::move(*BufferOrErr), &Context);
>>>    if (error(BinaryOrErr.getError(), Filename))
>>>      return;
>>> -  Buffer.release();
>>>    std::unique_ptr<Binary> Bin(BinaryOrErr.get());
>>>
>>>    if (Archive *A = dyn_cast<Archive>(Bin.get())) {
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits