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

Hi,

I’m writing with the hope of starting a discussion on how to improve the code-review process for clang-tidy patches. My experience so far as a developer has been rather frustrating lately. Here’s a summary of how it goes:

  • I put up a patch for review.
  • Very quickly (within hours) I get reviews from multiple people.
  • I address the comments.
  • Then reviewers go into complete silence for weeks or even months.

At this point, I’m honestly confused as to what’s happening:

  • Do reviewers get any notification when the patch is updated?
  • Do reviewers get any notification when I ping and tag them?
  • Do reviewers really don’t have time to at least communicate a rough ETA for review, or tag other reviewers? I understand people are busy, but I think it’s not unreasonable to expect a response (not review) within 1 week when people pings and asks questions.

Due to this, as a developer, I feel discouraged to contribute to clang-tidy. I fear others
might experience the same, leading to clang-tidy contributions being reduced over time.

How can we improve?

Looking forward to hearing your thoughts!

3 Likes

It seems I’m not alone alone here, see for example here: patch blocked for 4 months due to reviewers not providing feedback:
https://reviews.llvm.org/D128372

@njames93 , since you are the Code Owner in clang-tidy, would you mind sharing your thoughts?

I think reason is obvious: other developers are busy with something different. Some are currently working with different parts of LLVM Project (Clang, Clangd, Include-cleaner), some - outside. It may be reasonable if LLVM organization will sponsor/employ some maintainers (like Linux Foundation does), so they could have more time to work on its projects.

Still, the code owner is supposed to take responsibility for making sure reviews happen. If there is a code owner, and they aren’t doing the job, that’s a problem.

Denis and I been pinging D128372 weekly for months. I’ve also tagged @njames93 in the LLVM Discord, but no reviews have been made.

It’s incredibly discouraging to developers for projects to have code owners that are non-responsive for months on end. If their attention is taken up by other projects (which isn’t actually obvious given that @njames93 seems to have provided commentary to other clang-tidy contributions in recent times), then it’d be great to signal that instead of having contributors remain in limbo indefinitely.

But other quite active developers were also added to this particular merge request as reviewers, so it’s hard to claim that Nathan is bottleneck.

By the word, I always try to add developers with proven record of contributions to new Clang-tide patches on Phabricators. But this is not silver bullet in all cases.

For me it’s extremely confusing that I can get quick reviews within hours from many reviewers (which I’m infinitely grateful for!), and when everything is fixed and ready to merge, suddenly there’s not a single reviewer that can give any more feedback or approve the patch. Patches remain in limbo, merge conflicts happen, and in general they rot.

If the issue was that the Code Owner has no time, then he wouldn’t review any other patches, right? Therefore I’m more inclined to think that notifications are the problem here (either they don’t arrive, or they are ignored).

Sure, there are other reviewers, but are they all waiting for the Code Owner to give the final approval? There’s no guidelines that require so, but perhaps it’s not clear either that patches can be approved without the approval from the Code Owner.

It’s very sad to see patches either remain in limbo, or be self-approved by the patch owner, due to this.