[llvm] r211184 - Fix a memory leak in the error path.

Rafael EspĂ­ndola rafael.espindola at gmail.com
Mon Jun 23 15:01:07 PDT 2014


> That's the thing - I'm not entirely sure it is. I think it might be
> papering over the wart and just hiding the problem a bit deeper/more
> subtly. Though to be clear I'm not adamant/immovable/"over my dead
> body" about this... I'm just really not sure. You've certainly got
> more context (& more immediate interest in its maintainability - I
> don't work in this code much/at all) in this part of the codebase than
> I do, so I'm happy to defer to you. Just trying to express my concerns
> insofar as they might help choose a good direction forward.
>
> To jump off from Alp's comments - is there a longer term
> plan/goal/end-state you've got in mind, even if it's a bit vague?
>
> I'm not really sure I see anything better, long-term, than what I was
> suggesting - where a factory wrapper adds ownership via decoration,
> isolating the ownership aspect in a way that's orthogonal and
> non-invasive to the ObjectFile hierarchy, ensuring the caller has the
> right API, etc.

I am pretty sure that a solution with only std::unique_ptr members
instead of a pointer + boolean solution is an improvement. I also
don't see how it would hide the issue and make it harder to create
even better solutions.

I tested you suggestion of using references to std::unique_ptr in the
create functions and it works really well. There other improvements to
be made, like having the constructors themselves take a
std::unique_ptr and the create methods return a
ErrorOr<std::unique_ptr>> ,but these all are incremental improvements
that can be done in followup patches.

I will commit the first two. If you come up with a better solution,
feel free to post a patch.

Cheers,
Rafael



More information about the llvm-commits mailing list