[llvm] r214377 - Use std::unique_ptr to make the ownership explicit.

[Rafael Espíndola]

>>    if (!DWOFile)
>>      return false;
>>    // Reset DWOHolder.
>> -  DWO.reset(new DWOHolder(DWOFile.get()));
>> +  DWO.reset(new DWOHolder(DWOFile.get().get()));
>
> But looking at DWOHolder's ctor, it seems it was taking ownership,
> just via raw pointer - so your change is probably now causing a double
> delete. DWOHolder's ctor should take a unique_ptr by value, std::move
> into its DWOFile unique_ptr member, and this call site should be
> DWOHolder(std::move(DWOFile.get())) or DWOHolder(std::move(*DWOFile)).
> (also, I'd be inclined to change "DWO.reset(new DWOHolder" to "DWO =
> llvm::make_unique<DWOHolder>")

Good catch. Fixed.
Should some test be failing with double delete? According to

http://llvm.org/reports/coverage/lib/DebugInfo/DWARFUnit.cpp.gcov.html

It looks like we have 0 tests with a dwo file :-(

> How do you feel about using the pointer-like operations in ErrorOr? In
> this case that would be "DWOFile->get()" rather than
> "DWOFile.get().get()". There's many other cases where ErrorOr's op* or
> op-> could be used in this change - I won't call each one out
> separately.

I have mixed feelings. In my head an ErrorOr is not a pointer, but I
will admit to like the terseness of * versus that of .get().

> Also, (m)any cases where we need to pass some_unique_ptr.get() I see
> that as a great opportunity to use a reference parameter instead of a
> pointer parameter and just pass "*some_unique_ptr" - if the API is
> changed first, it actually makes the unique_ptr cleanup less invasive,
> since the usage is *ptr before and after the change. (in this case
> it'd be either "**DWOFile" or "*DWOFile.get()" if the function took a
> reference)

Yes, I fixed one such case in 214433.

>> -ErrorOr<ObjectFile *>
>> +ErrorOr<std::unique_ptr<COFFObjectFile>>
>>  ObjectFile::createCOFFObjectFile(std::unique_ptr<MemoryBuffer> Object) {
>>    std::error_code EC;
>>    std::unique_ptr<COFFObjectFile> Ret(
>>        new COFFObjectFile(std::move(Object), EC));
>>    if (EC)
>>      return EC;
>> -  return Ret.release();
>> +  return std::move(Ret);
>
> This std::move is unnecessary (since it's a local variable) and can
> inhibit NRVO.

I get

call to deleted constructor of
'std::unique_ptr<llvm::object::COFFObjectFile,
std::default_delete<llvm::object::COFFObjectFile> >'
  return Ret;

if I remove it.

> auto Obj = std::move(ObjOrErr.get()); // or std::move(*ObjOrErr);
>
> Not sure if that's better or not, just a thought. It could just be a
> reference to a unique_ptr too, since it's just there to provide a
> convenient alias. (or the existing uses of Obj->x could be replaced
> with (*Obj)->x). Preivously the separate variable was necessary/nice
> when the ObjOrErr wasn't managing ownership already. This recurs in
> several places where it might be worth considering dropping the
> intermediate variable.

Fixed in 214516.

Cheers,
Rafael