This is an archive of the discontinued LLVM Phabricator instance.

Differential D150813

[libc++][docs] Add note about RFCs for significant changes
ClosedPublic

Authored by ldionne on May 17 2023, 1:34 PM.

Details

Reviewers
Mordante
philnik
Group Reviewers
Restricted Project
Commits
rG87acf6ddc73d: [libc++][docs] Add note about RFCs for significant changes

Diff Detail

Event Timeline

ldionne created this revision.May 17 2023, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 1:34 PM
ldionne requested review of this revision.May 17 2023, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 1:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
ldionne added a comment.May 17 2023, 1:34 PM

See https://reviews.llvm.org/D148432#4350856

Harbormaster completed remote builds in B232694: Diff 523162.May 18 2023, 1:01 AM
Mordante accepted this revision as: Mordante.May 18 2023, 9:14 AM
Mordante added subscribers: var-const, philnik, Mordante.

Thanks, LGTM. I leave the final review to @philnik or @var-const .

philnik accepted this revision.May 23 2023, 10:55 AM

LGTM % nit.

libcxx/docs/Contributing.rst
31

I think just saying "that might have a significant impact on the user" is enough. I don't see the point of making an RFC for changes that are purely project-internal. We should just discuss this kind of stuff on the monthly meetup once we have that set up. If there are concerns about user interactions we can still write an RFC. (e.g. I don't think there is a point in asking whether we can rename _LIBCPP_CONSTEXPR_AFTER_CXXab to _LIBCPP_CONSTEXPR_SINCE_CXXcd)

This revision is now accepted and ready to land.May 23 2023, 10:55 AM
ldionne added inline comments.May 29 2023, 9:02 AM
libcxx/docs/Contributing.rst
31

@Mordante Are you fine with that? I am, but IIUC one of your concerns was that we often didn't do a RFC on Discourse for project-internal discussions as well, which could sometimes lead to a consensus that isn't clear to everyone.

I'd be fine with keeping the wording as-is or to changing it to only include decisions that potentially have significant impact on users.

Mordante added a comment.May 31 2023, 8:10 AM

LGTM!

libcxx/docs/Contributing.rst
31

I'm fine with it. I think it would be good to do Discourse too for large internal things. I still Discord is not great medium for these discussions. Good to start them but not good if it becomes a larger discussion.

One of my main issues is that proposals often don't have a conclusion whether we reached some sort of consensus. (But that seems to happen in more RFCs of LLVM.) Here I think having meetings will help to alleviate that.

However I don't feel we need to write all that down.

ldionne updated this revision to Diff 527160.May 31 2023, 12:26 PM
ldionne marked 2 inline comments as done.

Apply comments.

This revision was landed with ongoing or failed builds.May 31 2023, 12:26 PM
Closed by commit rG87acf6ddc73d: [libc++][docs] Add note about RFCs for significant changes (authored by ldionne). · Explain Why
This revision was automatically updated to reflect the committed changes.
ldionne added a commit: rG87acf6ddc73d: [libc++][docs] Add note about RFCs for significant changes.
Harbormaster completed remote builds in B235643: Diff 527160.May 31 2023, 7:41 PM

Revision Contents

PathSize
libcxx/
docs/
9 lines

Diff 527161

libcxx/docs/Contributing.rst

Show All 19 Lines
Before you start working on any feature, please take a look at the open reviews Before you start working on any feature, please take a look at the open reviews
to avoid duplicating someone else's work. You can do that by going to the website to avoid duplicating someone else's work. You can do that by going to the website
where code reviews are held, `Differential <https://reviews.llvm.org/differential>`__, where code reviews are held, `Differential <https://reviews.llvm.org/differential>`__,
and clicking on ``Libc++ Open Reviews`` in the sidebar to the left. If you see and clicking on ``Libc++ Open Reviews`` in the sidebar to the left. If you see
that your feature is already being worked on, please consider chiming in instead that your feature is already being worked on, please consider chiming in instead
of duplicating work! of duplicating work!
RFCs for significant user-affecting changes
===========================================
Before you start working on a change that can have significant impact on users of the library,
philnikUnsubmitted
Done
ReplyInline Actions

I think just saying "that might have a significant impact on the user" is enough. I don't see the point of making an RFC for changes that are purely project-internal. We should just discuss this kind of stuff on the monthly meetup once we have that set up. If there are concerns about user interactions we can still write an RFC. (e.g. I don't think there is a point in asking whether we can rename _LIBCPP_CONSTEXPR_AFTER_CXXab to _LIBCPP_CONSTEXPR_SINCE_CXXcd)

philnik: I think just saying "that might have a significant impact on the user" is enough. I don't see…
ldionneAuthorUnsubmitted
Done
ReplyInline Actions

@Mordante Are you fine with that? I am, but IIUC one of your concerns was that we often didn't do a RFC on Discourse for project-internal discussions as well, which could sometimes lead to a consensus that isn't clear to everyone.

I'd be fine with keeping the wording as-is or to changing it to only include decisions that potentially have significant impact on users.

ldionne: @Mordante Are you fine with that? I am, but IIUC one of your concerns was that we often didn't…
MordanteUnsubmitted
Done
ReplyInline Actions

I'm fine with it. I think it would be good to do Discourse too for large internal things. I still Discord is not great medium for these discussions. Good to start them but not good if it becomes a larger discussion.

One of my main issues is that proposals often don't have a conclusion whether we reached some sort of consensus. (But that seems to happen in more RFCs of LLVM.) Here I think having meetings will help to alleviate that.

However I don't feel we need to write all that down.

Mordante: I'm fine with it. I think it would be good to do Discourse too for large internal things. I…
please consider creating a RFC on `libc++'s Discourse forum <https://discourse.llvm.org/c/runtimes/libcxx>`__.
This will ensure that you work in a direction that the project endorses and will ease reviewing your
contribution as directional questions can be raised early. Including a WIP patch is not mandatory, but
it can be useful to ground the discussion in something concrete.
Pre-commit check list Pre-commit check list
===================== =====================
Before committing or creating a review, please go through this check-list to make Before committing or creating a review, please go through this check-list to make
sure you don't forget anything: sure you don't forget anything:
- Do you have :ref:`tests <testing>` for every public class and/or function you're adding or modifying? - Do you have :ref:`tests <testing>` for every public class and/or function you're adding or modifying?
- Did you update the synopsis of the relevant headers? - Did you update the synopsis of the relevant headers?
▲ Show 20 Lines • Show All 173 Lines • Show Last 20 Lines