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

[Eric Christopher via Phabricator via libcxx-commits]

echristo added a comment.

Hi Louis,

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


That's fine and I would encourage an abi code owners file if you'd like. That said, from the commit guidelines:

"Smaller patches (or patches where the developer owns the component) that meet likely-community-consensus requirements (as apply to all patch approvals) can be committed prior to an explicit review. In situations where there is any uncertainty, a patch should be reviewed prior to being committed."

Honestly, this should have been a fairly uncontroversial patch. __cxa_finalize is described in the itanium abi (and fwiw here as well: https://opensource.apple.com/source/Libc/Libc-1158.50.2/stdlib/FreeBSD/atexit.c ) as having a particular return type and is well documented as having that return just about everywhere I've seen. This is the sort of thing that likely-community-consensus very much applies towards.

Thoughts? Thanks!

-eric


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75795