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

[Fangrui Song via Phabricator via libcxx-commits]

MaskRay added a comment.

In D75795#1916168 <https://reviews.llvm.org/D75795#1916168>, @ldionne wrote:

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


I don't understand the argument. I reviewed the patch, not authored it. It was not me who pushed the commit. Even though, I think I should defend for this patch. It apparently met the likely-community-consensus requirements. I am not illiterate of these low-level stuff. I had checked **two standards** and **three implementations** before accepting it. Isn't that sufficient?

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

I agree. There are some components in LLVM which I don't want random commits coming in. I also agree that our bar for a permission bit is too low.

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

If you think my review for this patch is unqualified, please give concrete reasons. You may be discussing general policies regarding libc++ and libc++abi, but your words "What triggered me is that it’s at least the second time we’re calling you out for putting up a review" was apparently irritating and targeting me. If you wanted to deprive me of the right to approve, I would not agree. If there is any other place other than D52401 <https://reviews.llvm.org/D52401> where you think I offended you, please point it out. "What triggered me is that it’s at least the second time we’re calling you out for putting up a review"  made me very unpleasant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75795