[PATCH] D69674: [FIX] Make LSan happy by *not* leaking memory

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 20 11:25:26 PST 2019


dblaikie added a comment.

In D69674#1753862 <https://reviews.llvm.org/D69674#1753862>, @jdoerfert wrote:

> In D69674#1752604 <https://reviews.llvm.org/D69674#1752604>, @dblaikie wrote:
>
> > Looks like this was sent for review but no one reviewed/approved it? Generally, if something is sent for review, it's not great to submit it without review. (if you felt it needed review to begin with, time shouldn't change that)
>
>
> The review was intended to share the patch and validate the fix, that is why I didn't add reviewers. I will not open a review next time.


Validate in what sense? There didn't appear to be any discussion/validation from other developers here.

>> Any chance of using unique_ptrs here, instead of raw pointers and DeleteContainerPointers?
> 
> I can do that if you want me to. FWIW, it is a printer pass so I don't feel a strong need to improve performance here and I doubt there is "active development" on this part.

Ah, it wasn't so much about performance (I don't expect the unique_ptr solution to be any faster) but readability/maintainability - unique_ptr makes it harder to reintroduce bugs if someone adds an early return to that function or otherwise refactors it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69674/new/

https://reviews.llvm.org/D69674





More information about the llvm-commits mailing list