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

[David Blaikie]

On Fri, Aug 1, 2014 at 7:50 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>>>    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?

Well, it's UB - but yeah, not sure how that might manifest on our many
bots (valgrind or asan would catch this I'm fairly sure - not sure if
we'd get a good segfault or similar in the absence of those tools).

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

<3 coverage.

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

Lame. :/ Not entirely surprising - In all our actual cases llc outputs
one file with both dwo and non-dwo data and it's clang's driver that
runs a separate tool to split those up. So it's handy to have for
actual debugging but I don't think it comes up in normal LLVM test
cases. We should have a straight up llvm-dwarfdump feature test for it
then...

>> 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,

Yeah, I think of it in the same way as I think of boost/std::optional,
which uses pointer-like access.

> but I will admit to like the terseness of * versus that of .get().

Likewise. Ah well - we'll just play with it & see how various uses end
up looking. Not heinous to transform one to the other at some point if
we decide there's a strict improvement in one direction.

>> 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.

Hrm, right - this would be another of those conversion cases that'll
be fixed in 14 (or 17). Sorry about the incorrect advice.

The same options exist, and none is strictly better than what we've
got there...

>
>> 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.

Looks great, thanks so much! Great to be able to use that extra
variable/reference to rip through both layers (ErrorOr and unique_ptr)
and simplify the subsequent code.

(there was one case where the variable was introduced for just one
usage in which case I'd drop the variable - but I tend to write
rather... compact code, which isn't always optimally readable)

(& that does make me want to re-float the idea of having a dyn_cast
that takes a reference and returns a pointer (or Optional<T&>))

- David