[RFC] Improve code-review process for clang-tidy

I see this thread died on its own, well wake up, problem described in title is not yet fixed.

But real problem is that from a potential contributor Clang-Tidy project feels closed & dead.
If you look into all open reviews for clang-tidy you can basically feel frustration of contributors (that spend their time to improve this project).

Most common cases are like this:

  • someone submit for review some change of check to relax it or strengthen - change don’t get accepted because it does not follow infallible “C++ Core Guidelines”, or personal bias, if someone submit such change, then there is a target group for such functionality, put it under some configuration option and let it be.

  • someone submit some new check for specific functionality - even if change is welcome by reviewers they insist on re-implementing it as warning/static analyser but author may not feel conformable with those tools or submitter came with carrot salat, but reviewers want him to add there also tomato, cucumber and other functionalities, at the end author is frustrated and we end up with nothing.

  • someone submit path - it gets comments (more or less invasive), review gets sometimes accepted, or code gets to more or less acceptable level, but change isn’t submitted, or don’t get an accept. Even current code reviewers/owners have such changes open for months. If they cannot deliver own changes then it’s doesn’t look good.

  • someone submit path - and then nothing, excluding Eugene Zelenko none one even giving a crap about it.

7 years ago project that I work for, switch from some commercial tool (that didn’t understand properly C++) to Clang-Tidy. Sad thing is that over last years some checks that I had to implement internally were also implemented by other people, submitted for review and never get in.

I would really like to see this project as a viable competitor competitor for other tools, but currently it only scratches a surface and i don’t see any wiliness to change it by its owners.
Things like “Enhancing Clang-Tidy with project-level knowledge” are great, but they dead on arrival as none one wiling to say “lets go with it” so they stuck in limbo.

I hope that things will change to better and project will gain some colours.


There’s certainly room for improvement. People are welcome to step up and perform more code reviews – it does not require a code owner to provide good review feedback or to accept a change.

clang-tidy is more relaxed than Clang or the Clang Static Analyzer regarding what functionality it carries, but it is not a dumping ground for every possible diagnostic behavioral switch. Expecting there to be a known user base for the functionality is not unreasonable and adding a configuration option for everything is pretty well known to overwhelm users (especially given how easy it is to get the defaults “wrong” for any given option). We try to allow configurability where it makes sense, but it has to be justifiable.

I guess I’m confused as to what your concern is here. Authors and reviewers need to work together to make sure the functionality is implemented properly and will be useful to users. That sometimes requires changes the author may or may not consider to be necessary. As with any suggestions on a review, it’s a conversation involving both parties.

I think there’s sometimes a disconnect between patch authors and reviewers in terms of who lands the patch (we run into this in clang as well as clang-tidy). As a reviewer, we don’t always know who does or does not have commit privileges, but most of the folks we interact with are able to commit their own work. So our bias is towards assuming everyone will commit on their own once the patch is accepted. However, new contributors often don’t know to ask for someone to commit on their behalf. The end result is that sometimes things take longer to land than they should and other things fall through the cracks entirely. I wish we had a better way to address this (e.g., it’d be nice if there was some sort of badge next to reviewers/authors handles in Phabricator that showed whether they can/cannot commit so we can tell at a glance). Lacking something better, we rely on patch authors to say “hey, this was accepted but hasn’t landed, what needs to happen?” or for reviewers to notice on their own. It’s not ideal.

We have far more patch authors than we have code reviewers in clang-tidy, so it’s false to say that this is due to a lack of concern for the project. IMO, the only way the review process is going to improve in clang-tidy is for more people to step up and perform reviews.


There is a free-form field in phabricator on your profile. I added the text “I have commit access to clang-tidy” to my profile. Maybe if we all did this, it would help?

I’m guilty of not having much time to review code and have also been on the short end of the stick in getting stuff reviewed. I dropped a change from phabricator that hadn’t been reviewed in 4 years. I also agree that having patches in review for extended periods of time can lead to bitrot as the code continues to change underneath and rebasing can result in conflicts. As I’ve been on “extended absence” from clang-tidy related stuff I’ve been trying to work on, I periodically rebase against main and resolve conflicts – usually it’s the release notes where I’m adding a check. I just take it as par for the course, but it’s still less than ideal.

For new checks, would it be helpful to have a way of staging checks in an “alpha test” manner to allow people to try them on code bases? In the guidelines we suggest that you should run your changes against some big code bases in order to try it out first, but I know for my macro-to-enum check, I got very useful feedback by having someone run it against their code base and tell me how it went. Short of pointing people to your own fork of llvm-project and having your own releases, I don’t see a reasonable way of easily getting new checks into peoples hands for experimentation and feedback. (They can cone and apply your patch, but only the most interested devs are going to do that IMO.)

Maybe? I usually look at the person’s profile to see how long ago they joined and whether other patches they’ve submitted have been closed (and if so, who did the commit for them). Perhaps others also are already looking at phab profiles. However, it seems unlikely that folks will know to add this to their phab profile when they do get commit privileges, so it seems likely to skew towards a high false positive rate.

FWIW, we run into this same problem in Clang as well. In general, I think putting community efforts into a low-friction way for people to test experimental ideas without them being in the main branch will pay a lot of dividends. To me, one difficulty is: we don’t want to add code review pressures to maintainers for experimental patches, but we want experiments that show signs of success to have a straightforward path to being brought into the main branch without being a massive code dump to review. So there’s definitely a lot of open questions to think about.

We could add something to the getting started documentation for clang-tidy w.r.t. commit access. What I’ve seen before is that someone who doesn’t have commit access requests someone else to push their patch once it’s been accepted. I’ve done this a few times for other patch owners when I’ve reviewed their patches.

Agreed that we should document commit access information for clang-tidy – we could point users towards LLVM Developer Policy — LLVM 17.0.0git documentation for how they get it themselves. Maybe we should also update LLVM Code-Review Policy and Practices — LLVM 17.0.0git documentation to clarify acceptance of a change where the author might not have commit privileges?

After reviewing the history of Clang-tidy’s reviews and commits from the last year, I found that there were approximately 18 new reviews and 30 commits every month, with about 70% of reviews being closed within 18 days. The remaining reviews were usually not completed.

The majority of changes (by number of commits) made to Clang-tidy were done by njames93, LegalizeAdulthood, and carlosgalvezp. When examining the current state of things, it is evident that small fixes and improvements are typically approved, while more complex fixes and new checks remain in limbo.

Could it be possible to get help here from individuals involved in Clang-Fronted development ?
Their knowledge of the AST and ability to evaluate whether checks are being used effectively and correctly could potentially aid the review process.

I currently have around 8 checks waiting for review, and I’m unsure if it’s worth writing more if, after six months, I’ll have to continually rebase over 30 reviews.

At the end I cannot review/accept my own changes.

1 Like

Thank you for gathering that information, that’s useful to know!

This matches my intuition from watching the mailing lists, so thank you for verifying.

I think it’d be fantastic if Clang frontend folks were able to help out more on clang-tidy reviews, but it’s going to be a bit tricky to get people’s attention due to heavy review workloads there as well. Pinging specific people in the review with concrete questions may be an easier sell though – it’s usually faster to come in to answer a targeted question than it is to provide an actual review. That doesn’t necessarily solve the troubles you’re running into (folks in Clang may be hesitant to accept reviews in clang-tidy due to lack of familiarity with the tool internals), but it may at least help keep reviews moving forward when there are technical questions about Clang internals.

At the end of the day, the issue is one of not having enough folks who feel confident enough to review and/or accept changes in clang-tidy. You’ve been doing a great job of asking for help, but that help isn’t materializing, so we might need to consider alternatives.

Generally speaking, the community view on insufficient resources to maintain a project is that the project is no longer of interest to the community and we should consider removing it. However, I don’t think that’s a fair assessment of this situation – I see significant evidence of clang-tidy being used by users (searching around on sourcegraph shows it’s used by plenty of CI pipelines) and there are still people actively trying to maintain the project within the community. So I don’t think we’re to the “should we pull the plug?” discussions yet, but if the issue persists for long enough, it may come to that.

I’ve not seen evidence that any one module is taking up a lot of review cycles, but if there is such a module, we could consider deprecating and removing it to free up review time for other, better supported work. For example, my experience with almost every C++ Core Guideline check is that they’re incredibly resource-intensive when it comes to reviews, so if people are stuck on reviews trying to add new C++ Core Guideline checks, the reason might be because nobody in the community is willing to support the effort and thus it’s the module that’s the problem and not the tool itself.)

Because you don’t need a code owner to accept a review before landing it, I don’t think adding more code owners is going to help this situation. As best I can tell, the problem isn’t everyone saying “I don’t feel comfortable accepting”, it’s that we’re not getting the review feedback at all.

One thing we’ve not tried is asking corporate contributors if they have resources they could dedicate towards performing reviews. (We have folks contributing from Intel, Microsoft, Google, Apple, Facebook, and many others.) Similarly, we’ve never put out an open call to our users to ask them to help with reviews. Ultimately, I think a “Help Wanted Ad” might be our best bet to grow the reviewer base. We can put out a direct call for additional participation in the project on Discourse so people can share it around (internally, if their employers are interested in clang-tidy, or externally on places like Reddit or Hacker News), and hopefully that will get some folks excited and confident they can help. However, before we do that, we should make sure we have everything lined up to help ensure new folks succeed instead of get frustrated on arrival. We should validate that Getting Involved — Extra Clang Tools 17.0.0git documentation is still accurate, add information about the code review process or any other onboarding information, etc. FWIW. I can help with writing the Help Wanted post, but I don’t have the bandwidth to do the up front documentation legwork.

I agree that code owners are not required to land a patch, but I still believe we need at least one active code owner that is available for discussions, asking questions, and having somehow authority to make decisions as to how things should look like. Making sure things are kept together and consistent.

I have had patches of my own and reviewed others where I was not confident about higher level things, like if a check makes sense in a given module, changes to core ClangTidy classes, documentation, etc. In those I have requested input from the code owner without any success. If we are not going to have an active code owner I guess we need to accept that the quality of clang-tidy could potentially go down since there won’t be a centralized entity that “keeps things together” in a way that make sense.

That’s certainly the goal we’d like to achieve (and frankly, we need more than one active code owner for healthy projects – otherwise the code owner never gets to be sick, go on vacation, etc without causing disruptions).

Maybe my understanding of this situation is incomplete:

Some of these situations sound like architectural decisions where it’d be best to have a code owner input (things like changing core classes in significant ways) and others sound like situations where you’re looking for more input but not necessarily a fiat decision (documentation, where a check should live, etc). Is that accurate?

Generally speaking, in terms of major decisions, delays on review feedback are usually less of a problem because major decisions usually correspond to larger projects rather than smaller, immediately needed bug fixes. It seems reasonable for there to be plenty of “thinking time” for bigger decisions.

But if the pain is coming from a lack of input than lack of a decision, that’s a slightly different problem that I think is solved by having more reviewers actively involved, but they don’t have to be code owners.

Either way, I think we still have a resource problem in the clang-tidy community. I think we need more active reviewers to spread the workload around on typical patches, and we possibly need additional code owners for decisions. But I don’t think more code owners right now will help – if there’s only a handful of people semi-actively working in the community and most or all of them are code owners, then we’re basically in the situation where everyone self-approves anyway.

looking for more input but not necessarily a fiat decision

Yes. I’m speaking from the perspective of a code reviewer (which I’ve started to do on my limited free time). I encounter situations where I’m not confident on the changes - there I would like to have some more expert reviewer give the final approval. This would help me improve as reviewer too.

It seems reasonable for there to be plenty of “thinking time” for bigger decisions.

I would argue that it’s reasonable to have plenty of “discussion” time, more than “thinking” time. As a developer, I can spend lots of time thinking, but eventually I need someone to bounce ideas off, develop the idea, and essentially be sure that my ideas are being taken into consideration at all (and I’m not just casting them into the void). The problem I see is that ideas end up in limbo because there’s nobody (especially the code owner) to discuss them. I would rather have a quick “no, we don’t want this” than wait for eternity to receive an answer.

I absolutely agree that we need more reviewers, and the solutions you propose make sense. How could we address the issue of the code owner not being reachable (who hasn’t even commented on this very post, where he’s tagged)? What’s the point of having a code owner at all in this case?

Probably not that useful to say this but I’m going to do it anyway :)

I find reviewing stuff in phabricator a chore.

It’s not the reviewing itself that is a chore, it’s phabricator.

I know that’s not likely to change, but I wonder if it is contributing to the problem and if I am not alone in this feeling.

For me, clang-tidy is an on/off project not an ongoing thing. That explains why my participation is sporadic at best.

Personally, I get a kick out of my own clang-tidy checks showing up as an autofix option in my IDE. It’s me scratching my own itch and it’s fun.

I agree that we need a more actively participating code owner. I declined the offer because of my sporadic participation and having too many hobbies as it is :). I didn’t think it would be right to be added as a code owner if I couldn’t pay enough attention to it.

If we had multiple, rotating code owners, such that I only needed to pay attention 3 months out of the year, I could probably do that. That would mean 4 code owners, one per quarter. Is that a possibility?

1 Like

That’s definitely fair, I’ve been in the same situation before as well.

Yeah, we need both. Discussions need to happen when there are questions, and thinking needs to happen when there are complex decisions to be made.

Two points:

  1. We definitely want responsiveness from code owners on reviews (balanced against other needs, because code owners have jobs and lives outside of the community, too). It’d be great if code owners were also on Discourse, and Discord, and IRC, etc, but that’s not a requirement. (As a concrete example, I couldn’t get onto Discourse for quite a while after we switched to it, and I couldn’t get on Discord until a month or so ago, neither of which were really under my control.)
  2. We somewhat recently updated our code ownership across Clang and clang-tidy ([RFC] Proposed Changes to Clang's Code Ownership), and when I was trying to find folks willing to step up, @njames93 was the only person willing to be the code owner for clang-tidy. So the alternative is to have clang-tidy be unowned, but then we’re back to “is there enough community interest to warrant retaining the project?” kinds of questions.

I’ve heard this same feedback from others. Some love Phab, some hate it, others are somewhere in between. FWIW, I think there are still ongoing discussions about moving away from Phab and onto GitHub PRs, so it’s definitely possible we’ll be moving away from Phab to something.

I think there’s plenty of flexibility, so we can do whatever works for the clang-tidy community.

Really good point, I think we could get a lot more involvement from the community if we used Github PRs.

1 Like

Github PRs would definitely be an improvement for me.

Honestly, I have difficulty finding things to review in phabricator, mostly because there’s so many abandoned open reviews waiting for the author to do something that they’re never going to do.

1 Like

FWIW: When I took up the ownership I had a lot more time, but soon afterwards I got a new job which severly restricts the amount of time I’m able to to spend working on clang-tidy.
Currently I do what I can when I have time but it’s definitely too much to rest on my shoulders. If someone else wants to share ownership with me I’d be more than happy with that solution.
If @carlosgalvezp wants to share the torch bearing I would be happy

The discussion about moving to Github PRs, while likely an improvement is not something I really think can be done in isolation, it would need to be the whole of LLVM(or potentially just the clang-tools-extra subproject) that would need to migrate away from Phab before we can use Github for clang-tidy

1 Like

That doesn’t sound like a Phabricator issue though, that would apply equally to GitHub PRs if adopted?

1 Like

I’m willing to chip in if we can get at least a 3-way split. Like @njames93 I have “other irons in the fire” so to speak. The reason I didn’t accept before is that I didn’t want to pretend I had more time than I do. The reason my “YES” means something is because I have the courage to say “NO”, if that makes sense :).

1 Like

I’ve seen enough reviews from Carlos that I’d have no trouble supporting his nomination if that’s something he is interested in doing.

+1; clang-tidy (and really, all of clang-tools-extra) should continue to follow the same community processes as Clang and LLVM to the greatest extent possible. My comment about the potential of switching review tools was a comment about the community as a whole considering the switch.

FWIW, I’d also support your nomination as code owner.

1 Like