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

[David Blaikie]

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