Thank you @AaronBallman for this initiative, and for stepping up and volunteering to take over as fallback Clang owner! These changes look good to me and I’m happy to see that Clang is in good hands going forward.
3 Likes
Great initiative, thanks for making this happen!
1 Like
I’m good with this - I was trying to keep the driver going and any other help here is great. You can leave me as a backup if you’d like. 
1 Like
Thank you Richard! Your hard work and incredible depth of knowledge have helped fundamentally shape Clang into a great compiler, and your kindness and patience while doing so have set a fantastic example to try to live up to. Thank you for everything, and I hope we still see you around from time to time!
3 Likes
Thank you for the offer! We chatted about this off-list and decided against it for the moment but have a plan in place for the future.
Thank you for the support and your offer! Eric and I spoke off-list about this and we have a plan in place for the future.
+1.
Richard you’re a legend and I really hope you can find time to poke your head in around here whenever you have a chance.
2 Likes
I believe this should be a larger trend into LLVM and the other projects, but I am glad to see this already happening in Clang. +10 for this initiative!
Throughout time, I have actively worked on and acted in the expectation of a code owner for different parts of LLVM. We also have enough cases of people who are very active but are not listed in the code owners file, and people in the list that haven’t worked in LLVM for ages.
I have tried to bring this to light a number of years back, but the feeling was that “it doesn’t matter much what’s in the list because we all know who work where”. This could be true back then, but with the increase in the number of projects, people and code contributions, it gets harder and harder to know who’s active and who’s not. In the end, if the list doesn’t matter, why do we have one?
Your interpretation of “code ownership” seems to be the same as mine: people who actively work on a project and that have taken (by work, not nomination) responsibility for its quality, today and in the past (emeritus). But the use of such a list to the community is to know who to contact, who to add to reviews and who to ask for help, so the list need to be fresh.
The need to have more than one person per area and the ability to have more than one area per person makes a lot of sense, too.
Perhaps next we should have a wider discussion on:
- What exactly do we mean by code owner? Rights (ex. Github) and responsibilities?
- How do we split sub-project ownership to all projects? (like you did well with clang)
- How do we know when to add a new code owner? If people request, who approves?
- When do we remove owners? if we have multiple, do we even need to?
- How do we guide users to know who to contact for reviews/questions/access, etc?
I’d suggest this to be an organic list, where people are never removed, but moved into a “retired/emeritus” list for each major component (like you did with Richard), so that people know not to bug people that don’t work anymore, but we still keep them there, for historical records.
1 Like
Is it appropriate to mention target owners somewhere? These are mostly for LLVM work, which is not the focus of this thread, but there are a few places in Clang where target-specific things live (Basic/Targets, Driver/ToolChains). Maybe a note indirecting to the LLVM code owners would work.
2 Likes
Having a code owner for targets in the FE makes some sense from a “who should review changes” perspective, but changes to targets are so infrequent that I’m not certain we need an owner for it so much as for me to be aware of who to add into the review on the occasions when a target is being modified in some interesting way (instead of just refactoring or whatever). It’s worth putting thought into though, so thanks for bringing it up!
FWIW, I’ve had the same sorts of questions kicking around in my head as well.
To me, a code owner serves two main purposes: 1) Performing quality code review in a reasonably timely manner for some part of the code base (or oversight of someone else is doing quality code review), 2) Making “final decisions” when there’s a need. #1 happens continually, #2 happens about once a year, so code review is the biggest reason to have a code owner.
My current thinking on how to split duties up (which is still evolving as I get more feedback from folks) is that code ownership is really about community need:
- Is the area critical to shipping Clang (templates, standards conformance, cmake, etc)?
- Does the area need significant oversight to ensure the whole product remains sustainable (ABI, attributes, command line options, etc)?
- Is the area a conceptually self-contained, very large extension or feature (sycl, cuda, opencl, openmp, concepts, modules, constant expressions, etc)?
- Is the area a (conceptually) separate tool with significant user-facing interactions (clang-tidy, clang-format, the static analyzer, etc)?
In terms of adding or removing owners, I think it makes sense to add folks when we identify a community need (see above) and have agreement on a good owner or co-owner, and I think we should remove folks who wish to step down or who become inactive in the community for an extended period of time and cannot be reached (there’s not a lot of benefit to listing people as owners who won’t be doing the duties).
However, I’m still thinking about these kinds of things and so none of this is really firm for me. Having a broader community discussion around this sort of thing seems very appropriate, though I’m not certain what the expected result of the discussion would be. Are you hoping we’d document something about this, or are you mostly hoping to see if the community is largely on the same page about responsibilities (or both, or something else entirely)?
1 Like
Thank you for driving this initative @AaronBallman, this is a huge step forward for Clang, and I’d love to see similar efforts in other parts of the project!
These are great questions, could we move this discussion to the LLVM Project category so folks beyond the clang subproject are more likely to participate?
Thank you all!
-Chris
1 Like
nice, thx!
1 Like
While you’re at it, it would be great if you could add Herald rules to automatically assign the correct reviewers to patches. Maybe even a better option would be to create projects which are actually owning the code. These projects could be automatically set as reviewers. That would allow anybody to subscribe to the parts they are interested in and also show who is allowed to approve a patch.
For example, for libc++ we have a libc++ project on phab which automatically gets assigned to any patch that touches libc++ code. Anybody who is interested in the libc++ development can add themselves as a watcher and the members are allowed to accept patches for libc++.
This allows us to have Louis as the official code owner for any decision making, but have quite a few other people that can approve normal patches.
I’m not sure if this would be a good way to handle clang code reviews, but it works very well for libc++.
1 Like
This is a really good idea! We did this with the clang language wg as well, and it’s worked okay so far (not gotten a ton of extra review from it, but it’s new so it might take a while for folks to take advantage of it).
I’d be wary of signing up all the code reviewers for Herald rules automatically; I think they should opt into whatever mechanism works best for them to track reviews (for some folks, the Herald rules would basically be “touches any part of the project” so it doesn’t work for all roles). But I think we should strongly encourage some form of automation where possible.
Thank you for all the feedback! I’m seeing no opposition to the proposal, so I think this RFC has been accepted. I’ll leave the review open at ⚙ D132550 Changes to code ownership in clang and clang-tidy for a little while longer in case people have comments about the changes I made to the layout of that file, but I expect to commit it early next week. If you have concerns, now is a great time to voice them.
+1, thanks Aaron
1 Like
I’ve committed the changes in 3c604e9f15606fa395b088ca341a232ffda2bb7d, thank you to everyone for the feedback!
2 Likes
Overdue