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

I’d be honored! Unfortunately I’m pretty busy at the moment with full-time job and a newborn at home and I’m already having a hard time catching up with existing reviews. But I can definitely consider it again when I get some of my free time back :slight_smile:

That could be a really good solution actually, increasing the likelihood of having someone available within reasonable time.

Hello everyone.

A release has passed, and I believe it’s a good time to unfreeze this discussion.

Some statistics for a start:


Calculated based on “clang-tidy” label/tag/title, may be a little bit inaccurate.

Overall Clang-tidy 17 release timeframe wasn’t too shabby, 20 new checks were squeezed into that release, together with ~55+ fixed & improvements. However, there’s room for improvement.

Still amount of open issues went down from a 691 to a 575, Carlos Galvez (@carlosgalvezp) did here great job by sorting out some old issues.

André Schackier (AMS21), Chris Cotter (@ccotter), and Mike Crowe (@mikecrowe) have collectively contributed over 45 commits, making them some of the most significant contributors, even though they may not have commit rights.

When we closely examine the data, a few warning signs become apparent:

  • We’re barely keeping up with fixing/sorting out new issues.
  • Only five individuals delivered more than 10 commits this year, with just seven pushing 5-9 commits, while all other changes were more like one-time contributions.
  • As for code reviews, there aren’t many individuals who can currently accept reviews; in fact, it’s primarily Carlos and me.

For me, the main bottleneck at present is the code review process. To illustrate, it took months to receive a code review and approval for certain new, relatively simple checks. Even after posting a thread on Discourse, Shivam Gupta (@Shivam) was essentially the sole respondent, playing a pivotal role in getting the majority of those checks integrated into Clang-tidy 18.

Fixing existing checks is quite challenging. When Carlos reviews them over the weekend, things tend to go smoothly, but it still takes about 2+ weeks. Involving the check’s original author can be hit or miss. Sometimes, it’s a quick process with simple fixes, and the author is content. However, at times, authors remain convinced their creation is flawless and doesn’t require any adjustments. In other instances, discussions stray towards adding extra, non-critical functionality instead of delivering a small fix. These challenges have led to burnout and have me questioning my involvement in the process.

On the flip side, attempting to fix a check often reveals that the check’s implementation is outdated. It uncovers additional issues beyond what was initially reported, and essentially, the check requires more than just a few adjustments; it needs a small refactoring. However, in such cases, reviewers typically request that each change be submitted in a separate code review. This approach can extend the review process to the point where it takes months, and there’s a significant risk that the changes may never be accepted.

This highlights the primary issue with code reviews. From a contributor’s perspective, the most crucial factor is delivering changes promptly. It’s perfectly acceptable to receive feedback like ‘modify this and that, and it can be accepted.’ However, when reviews are delayed for weeks with no response or apparent interest, it saps the motivation. The question arises: what’s the point of working on additional fixes or improvements if there’s no guarantee they will ever be delivered within a known timeframe?

The current Phabricator system presents challenges, as it’s easy to miss reviews. I rely on subscriptions for changes in the clang-tidy directory and email notifications just to stay updated. With the significant volume of code reviews, these few clang-tidy reviews often become nearly invisible. I’m eagerly anticipating the transition to Github Pull Requests, primarily for the improved visibility through labels. Additionally, it seems that some individuals are already showing interest in new clang-tidy issues, even if they are not actively involved in reviewing changes at the moment. This transition may attract more developers who are eager to accept clang-tidy reviews.

If that won’t help then maybe some unconventional solution will be needed.

Has anyone encountered similar challenges in other parts of the LLVM project or in different open-source communities?

Best regards,
Piotr

3 Likes

Thanks, @Piotr for mentioning :smile:

It was a great experience in reviewing and knowing clang-tidy checks internals.

I would encourage anyone and everyone irrespective of the project they are working on, involve in clang-tidy development, it certainly will improve their understanding of modern C/C++ and steadily help them in writing better code.

Thanks for the post @PiotrZSL ! Indeed it’s a challenging situation. I really wish I could dedicate more time to review, unfortunately I currently don’t have bandwidth for other than small fixes.

However, in such cases, reviewers typically request that each change be submitted in a separate code review. This approach can extend the review process to the point where it takes months, and there’s a significant risk that the changes may never be accepted.

It’s good to keep in mind that the review process is not linear. If 1 patch takes 2 weeks, it doesn’t mean that 2 patches must take 4 weeks. For example, as a reviewer I feel confident in quickly approving an NFC patch that just modernizes/refactors the code without touching the tests. However if the NFC changes are mixed with functional changes, it’s hard for a reviewer to understand exactly what change in code leads to the change in tests - there’s a lot of noise in the patch that ultimately leads to either delays in reviews or simply not willing to review at all. The smaller and more focused the patch, the more likely it is to be reviewed quickly and with quality.

I agree it’s hard to get proper notifications when asked specifically to review, as they easily get lost in the email notifications about new patches in clang-tidy - maybe it’s possible to tune the mail filter to distinguish both, or simply remove the notifications so that only those where an explicit review/tag is requested pop up.

I really look forward to the Github PR flow, I think it will greatly improve the process and will lower the entry bar to review and contribute to the project!

My only excuse is that my available time is sporadic as I contribute here as a hobby :).

I don’t know if we resolved to adopt a round-robin code owner policy in which I would participate as code owner for something like 4 months a year where I would make a concerted effort to pay more attention LOL. I know we discussed this idea a while back.

I think switching to pull requests and ditching the weird phabricator thing (honestly, never got comfortable over there) should smooth out some bumps.