I’m looking for some advice on how to deal with a developer whose code consistently falls below a reasonable standard.
I’ve been regularly added as a reviewer (either explicitly by the developer or by my automated rules because they make changes in the areas of code I consider myself fairly knowledgeable and try to review most things) by a specific developer to their patches. Their code quality is routinely poor to say the least. Some examples of poor quality are 1) repeatedly failing to conform to LLVM coding standards; 2) code duplication; 3) a failure to properly handle Error/Expected return values (often ignoring them/using consumeError inappropriately etc); and 4) using static local variables for storing state between calls to a function (which is very thread unsafe and introduces unreasonable code coupling).
Yes, we were all beginners at one time, and so I’ve been happy to correct and point out these issues when they’ve cropped up in the past, plus even more experienced developers occasionally slip up. However, this developer consistently is making these mistakes over dozens of patches over the course of multiple years now and is not showing any obvious signs of imrpovement (when I think they have improved, they start making the same mistakes again). Even a very junior developer should have been able to take my comments and corrections on board by now and improved. If they didn’t and LLVM were a company’s internal codebase to which they were employed to work on, I suspect they wouldn’t still be doing the job by now.
I’ve already pointed out to the developer that I can’t keep spending all my available reviewing time correcting their repeated mistakes, and have asked them to make sure they review their own code before putting it up for review, in the hopes that this will weed out some of the simpler mistakes, but this hasn’t really resolved the issue, and it’s got to the point where I’m actively choosing not to review their patches for several days because I can’t face trying to deal with their code anymore.
I’m deliberately not naming the individual or pointing at a concrete review, because I want to help them rather than shame them, but I’d appreciate it if anybody has any advice on what can be done to improve the situation.
Someone unwilling (or possibly unable, unlikely as that seems) to learn is indeed a trial.
I suggest that you stop correcting their repeated mistakes in detail. Instead, say something like “This patch has issues similar to (other patches you have commented on), please correct those issues first and then we can proceed.” Other canned responses are also possible, and you might want to enumerate specific problems rather than say “similar to other patches” which might not be direct enough for the author to take action on. Citing links to the style guide or other references might also help. The point is, being a canned response, it takes not much effort on your part (beyond the effort to write the first one), but you aren’t ignoring them.
Then mark the review as “requires changes.” Other reviewers should take that as an indication that you are serious and not approve without your complaints being addressed.
As you have already tried to resolve this issue with the person, and not made much progress, I would suggest asking a colleague to also review the work, to ensure there is no unconscious bias and that they agree with your assessment of the situation.
I would then discuss it with the LLVM dispute resolution board to find out what the possible way forward is. I guess one extreme option might be to simply not accept any more patches from that person, but LLVM is very much an “inclusion” type group, so hopefully the person in question can be motivated to improve.
I am a Linux kernel developer, and the expectation there is that the barrier to getting a patch accepted is pretty high (quality wise). I don’t see why LLVM should not set the same high quality barrier.
+1 to this, I would even go further: the person may benefit from a mentor: waiting for the code review time is “late” in the process. They may benefit from help and review along the way before getting to code review.
Thanks for bring up this topic! Reviewer time is a scarce effort and it’s good to see discussions about this topic. Companies should keep in mind that such behaviors harm their reputation.
Some companies may post patches after a round of internal reviews, at least for some patches. A lot of issues can have been caught even if the internal reviewers are not a domain expert. Informing the author or their colleagues may be helpful. Ideally, the colleagues can step up and proactively do this for patches that may negatively affect upstream reviewers’ time.
While risking to derail this topic, I am not trying to…
We should have better tooling. Our style checkers are too strict, and stuff like static variables, comment structure, documentation, etc. are not reported by tools we employ, or tell people to employ, at least as far as I know. Maybe we should have a tool/service to give you reasonable feedback on code. Style and other things are pointed out with explanation. Lot of work, and slightly too late for GSoC. We should keep it in mind though.