Re: automated coding style tool

[Simon Marchi via Gdb <gdb@sourceware.org>]

On 1/17/25 10:13 AM, Joel Brobecker wrote:
>> I don't think adding this tool is going to be the magic bullet everyone
>> seems to think it is.
>>
>> As a reviewer you'll either still need to learn the style in order to
>> spot when a contributor has not running the formatting tool.  Either
>> that, or you need to apply each patch locally and the check run the
>> formatting tool to check the formatting is correct.
> 
> I have experience with a project where developers were asked to install
> a pre-commit check that would run the formatting tool, but where we did
> not have (yet) a pipeline to verify the formatting at submission time.
> This project had a lot of developers, some of them very occasional.
> 
> It's true that, at first, we had a number developers forgot to install
> that pre-commit hook, and ended up submitting ill formatted code.
> 
> What I noticed, at least for myself, is that with time you start
> developing an eye for what the expected formatting is like. And when
> I noticed something odd, I'd just ask if they installed the pre-commit
> and ran the formatting tool. If they say no, I pointed them to
> the directions, and asked them to retest and resubmit. Ultimately,
> we mostly converged.
> 
> And for the cases we didn't notice at review - no big issue.
> The formatting shouldn't be horrible of the reviewer would have
> noticed it, so no great harm -- certainly no worse than right now.
> And later on, the next developer touching the same file ends up
> noticing that the formatting seems wrong. It's a bit of a pain
> for them to first run the formatting tool and push a pure-reformatting
> commit.  But we tell the contributor who forgot, they install the tool,
> and normally we eventually converge.
> 
> Another things we can imagine doing is a nightly job on sourceware
> that runs the nightly snapshot through the formatting tool, and
> sends an email with a list of files that are not conformant.
> 
> Where I think it might be painful is which version of the tool
> to use. Different versions will likely have slightly different
> results in terms of formatting the code. You might want to require
> a specific version and check for that version, to avoid different
> users having different ways of formatting the same code.

I agree with all the above.

We will definitely want to choose a specific version of the tool.  If we
end up using clang-format, that version will likely not be available out
of the box for all distros, but there are other ways to get it.  I don't
know how it works on Windows though, since I don't develop on Windows
much.  We can cross that bridge when we get there.

>> I think if GDB could just move away from the mailing list and each users
>> pushes their own patches model, over to a pull request style approach,
>> then we could potentially have _real_ automated checks, e.g. checking
>> the formatting.  Then we have an _actual_ win.
> 
> Seconded.
> 
> In a model like this, you don't have to ask. You run a job that
> checks it, and you know if it's conformant or not.

I don't agree with Andrew, in that even without a proper pre-merge CI,
you do save time by using a tool to format the code, over doing and
reviewing it manually.

Of course, like Joel said, ill-formatted code will certainly get
committed at some point, but the nightly job will tell us.  It's a quick
fix - one person pushes an obvious patch - and we carry on.  Much less
time overall than pointing out formatting issues one by one (for the
reviewer) and fixing them by hand (for the author).  This is what we do
with our Python code and it works well, I think.

I suppose it would also be possible to write a server-side commit hook
that checks whether the formatting of the affected files is correct, and
reject the push if not.

Simon