[libcxx-commits] [PATCH] D118278: [libc++] Add documentation about the libc++ review group

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: var-const.
ldionne added inline comments.


================
Comment at: libcxx/docs/Contributing.rst:51-52
+
+After uploading your patch, you should see that the "libc++" review group is automatically
+added as a reviewer for your patch. Once the group is marked as having approved your patch,
+you can commit it. However, if you get an approval very quickly for a significant patch,
----------------
Quuxplusone wrote:
> If you upload the patch via the web interface, you should add "libc++" to the Reviewers field, and also add "libc++" to the Tags field. Adding "libc++" to the Subscribers field is unnecessary. (At least, that's what I've always done, and nobody's complained at me yet!)
Above, we ask that folks upload patches from the command-line using `arc`. We could change that if you feel strongly about enabling that use case, but TBH I feel that we gain more from new contributors just using `arc diff` (and everything happening as it should) instead of trying to use the web interface. We've had hiccups with that before (and I'm even surprised it works nicely for you).


================
Comment at: libcxx/docs/Contributing.rst:60-62
+rule -- for very simple patches, use your judgement. The "libc++" review group consists
+of frequent libc++ contributors with a good understanding of the project's guidelines --
+if you would like to be added to it, please reach out on Discord.
----------------
Mordante wrote:
> Quuxplusone wrote:
> > It would be good to find a way to indicate who belongs to this group; otherwise
> > (1, the reactionary complaint) someone who considers themself a "frequent contributor with good understanding" may assume they are permitted to accept-as-libc++, when we don't want them to
> > (2, the more constructive complaint) someone who's definitely a libc++-accepter, such as myself, may see that a PR has been greenlit by another contributor, but I'm not sure whether that other contributor counts as a libc++-accepter, so I'm not sure whether I should accept-as-libc++ or not.
> > 
> > Maybe we don't want to put the names right here, either because it'll bit-rot (but maybe we should maintain the list!) or because we're too modest (but we shouldn't be!). But it'd sure be nice to do //something// explicit.
> > It would be good to find a way to indicate who belongs to this group; otherwise
> +1 but not blocking for this review.
Oh, I guess I'm the only one aware about this, but the members can be seen here: https://reviews.llvm.org/project/members/64/

I'll add a link! And while we're at it -- it is worth mentioning that both @var-const and @philnik are now part of that group. Note that as we expand the group, my goal is to reduce the review bottlenecks without decreasing the code+tests quality or losing a coherent overall direction for the library. I think that can be achieved by people reviewing stuff and approving only when they are confident that they understand the area of the patch and its implications, and reaching out to the right people when necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118278