[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#1915841 <https://reviews.llvm.org/D75795#1915841>, @ldionne wrote:

> In D75795#1915821 <https://reviews.llvm.org/D75795#1915821>, @lebedev.ri wrote:
>
> > In D75795#1915802 <https://reviews.llvm.org/D75795#1915802>, @ldionne wrote:
> >
> > > Folks, it's not okay to commit changes to libc++ without having a libc++ or libc++abi code owner review them. If you wait for us to review the change and a review is not coming, you can ping the patch. But you can't commit it just because 2 business days have passed and we haven't said anything.
> > >
> > > @MaskRay This isn't the first time this happens. The LLVM community is very loose by giving global commit access to everybody, but we're expecting people to act in good faith and not bypass the usual review process.
> >
> >
> > Was phabricator-friendly approach considered?
> >  Why not have a herald rule to auto-put someone from libc++ as blocking reviewer on new libc++ patches instead of such trigger-happy reverts?
>
>
> @EricWF  is automatically added to these reviews. It's not marked as "blocking" though, since I guess that would create a huge bottleneck on all reviews.
>
> Usually, someone who has several libc++/libc++abi commits will chime in and review these changes. I'm thinking about folks like @compnerd , @phosek , @mstorsjo, and there's others. Whenever one of these folks sign off, I think most people are comfortable and you won't see anyone complain. But this isn't the case here.
>
> This has rarely been a problem so far, but I guess we should consider a `libcxxabi/CODE_OWNERS.txt` file (@EricWF and I were talking about it just now).


`llvm/CODE_OWNERS.txt` lists @mclow.lists as the code owner of libc++, but in reality, many of us know that the triumvirates exists. You and @EricWF can feel free committing and can approve others' changes. By the way, I very much appreciate your reviews. You are working really hard to make libcxx better.

I shall put up one point, libcxxabi is not libcxx. Well, we all know that the number of commits cannot summarize one's contribution to the project, but it at least can commute some interesting facts.

  179  Howard Hinnant
  135  Eric Fiselier
   65  Erik Pilkington
   62  Saleem Abdulrasool
   53  Petr Hosek
   36  Marshall Clow
   34  Nick Kledzik
   29  Dan Albert
   29  Louis Dionne
   27  Jonathan Roelofs
   24  Logan Chien
   23  Nico Weber
   17  Richard Smith
   14  Shoaib Meenai
   11  Asiri Rathnayake
    9  Joerg Sonnenberger

I think people will not complain if any of `@howard.hinnant @erik.pilkington @compnerd @phosek` approves a change. I have close to zero contribution to libcxxabi, I don't deny. However, for this case, I am very confident that Ryan was making the correct change (my only complaint would be [1]). I wrote enough justification why it is correct. 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.

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

[1]: it'd be nice to strip some Phabricator metadata tags in the commit description:

  arcfilter () {
          git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75795