[libcxx-commits] [PATCH] D75795: [libc++abi] Change __cxa_finalize return type to void

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D75795#1915980 <https://reviews.llvm.org/D75795#1915980>, @MaskRay wrote:

> In D75795#1915841 <https://reviews.llvm.org/D75795#1915841>, @ldionne wrote:
>
> >
>
>
> I shall put up one point, libcxxabi is not libcxx.


No, it’s not, but they are intimately tied.

> I think your action is a bit over the top. People probably don't mind if the realm of libc++ is extended to libc++abi as well, but unnecessarily reverting a change to make it your territory is just not nice. If there is any omission from this patch, please just state it. You could still do a post-commit review.

My goal was very much not to make libc++abi my territory by reverting your patch. I’m actually much happier letting it be other people’s territory as much as possible.

I have to agree my reverting was prompt and I could have asked you to do it instead (or commented on this review). I apologize for that, my intent was not to come out as territorial.

What triggered me is that it’s at least the second time we’re calling you out for putting up a review and committing without giving time to respond. Last time I remember having a discussion with other maintainers on Slack and we were all annoyed, even though I was the one to revert your commit (and hence take the flak).

> 
> 
>> There is a strong expectation that authors respond promptly to post-commit feedback and address it. Failure to do so is cause for the patch to be reverted.

Post commit reviews don’t work very well in a world where I don’t even have time to properly review everything in my queue. I can’t speak for other maintainers, but personally when something is committed, I wont review it again unless I need to or have a specific concern about it, because I just don’t have time.

So, from my point of view (again, I can’t speak for other reviewers), post-commit reviews for libc++ and libc++abi don’t work. Just today I spent significant time reading through code that was committed 6 months ago in libc++abi without review and broke something on a platform I maintain. I learned about the existence of that change just today, cause I missed the original commit email.

(Sent from phone)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75795