[FR] Run clang-format on C files

[SteveBronder]

I'm reading through rbindlist.c and there are several lines like

    for (int j=0; j<thisncol; ++j) { int tt=length(VECTOR_ELT(li,j)); if (tt>maxLen) { maxLen=tt; whichMax=j; } }

It would be nice to run clang-format over all of the c files in src just to make these lines more easily parsable.

Eigen and the Stan math have done this and there's a lot of discussion around the pros and cons. The main con being that you have one hard break in your git, but that can be resolved by adding a .git-blame-ignore-revs file that ignores the reformatting. A .clang-format file can be included to modify any of the defaults as the team wishes

[eddelbuettel]

I have no vote here but consider this a strong against not the principle of a code formatter but the default style of clang-format. It a nutshell, it is very 'narrow and vertical' and a very strong departure of the style used in for example base R and here which tends to be more 'horizontal'. It would be nice to agree a desired format before point-blank jumping into the fury of reformating a code base. FWIW the only 'project in the vicinity' that I am aware of which actually defines a custom clang-format input file is duckdb in this file.

[SteveBronder]

Definitely +1 against the default. The full set of formatting options for clang-format are at the link below. Eigen for example (link) uses 120 width columns instead of the default 80.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[eddelbuettel]

So does duckdb. I would love to see a decent gallery of effects of different settings. Do you know of one?

[MichaelChirico]

I'm definitely in favor of consistency, but agree with Dirk that using defaults is not likely to suit our codebase.

The way to go about this IMO is to incrementally enable rules -- this will likely require a dedicated contributor willing to spend time building such PRs. Rough outline:

1. Demonstrate the effect of a rule on a representative file, then maintainers can comment on whether it's suitable
2. Apply the rule globally, possibly rolling back if some cases are exposed that go against the initial discussion
3. Enable the rule in our .clang-format file & make sure CI will error on incremental PRs that violate the rule
4. Repeat. Steps 1&2 might be combined for rules that only impact a small number of lines.

That said, this will not be a priority until we can burn down some of the existing PR queue -- as Jan has emphasized a number of times, low-impact changes that create merge conflicts with existing high-priority PRs will just create maintainer toil that we'd rather avoid.

[SteveBronder]

I'm definitely in favor of consistency, but agree with Dirk that using defaults is not likely to suit our codebase.

I also agree

The way to go about this IMO is to incrementally enable rules -- this will likely require a dedicated contributor willing to spend time building such PRs. Rough outline:

This feels like a lot of effort (there are like a 100 rules). For Stan we just turned off all the rules, skimmed the list of rules we thought would be good and added them. Adding a linter is a lot about agreeing on a style and you really need a main contributor to know what the team wants and to get agreement

That said, this will not be a priority until we can burn down some of the existing PR queue -- as Jan has emphasized a number of times, low-impact changes that create merge conflicts with existing high-priority PRs will just create maintainer toil that we'd rather avoid.

Yes I think waiting till after a major release would be a good idea. It's nice for future contributors at the pain of current PRs

[eddelbuettel]

Did you just conflate / confuse a discussion about clang-format (aka "bad enough as it is with its default") with the R linter whose defaults are truly [..... redacted on advise of counsel ...] ?

[MichaelChirico]

Noting here some rules we should definitely apply when we get around to it, please feel free to edit this comment

• return(.); --> return .;

[SteveBronder]

alignescapednewlines: I like ENAS_Left but ENAS_Right is also fine
AllowShortBlocksOnASingleLine Setting all the AllowShort* to SBS_Always let's you keep one liners simple
ColumnLimit: Eigen uses 120 which is nice, though personally I prefer 100. I use a horizontal screen and 120 can run over a bit with my normal font size
InsertBraces: For the initial PR this should be run once separately as it can lead to malformed code
Pointer/Qualifier/ReferenceAlignment: I like Left personally but this one is up to data.table's style

[eddelbuettel]

@SteveBronder That is helpful but you only show where you would pull changes from but now how. Can you add 'best guesses' or preferences for the suggested values of these?

[davidbudzynski]

So does duckdb. I would love to see a decent gallery of effects of different settings. Do you know of one?

There's this setting configurator that allows you to tweak different styles and see what it does to the code.

[MichaelChirico]

cc @aitap here is the open issue on using more standard tools for linting & code quality in C.

Status is a plan to incrementally enforce rules gradually, at least as long as our PR queue is so long, to balance the burden of updating merge conflicts in longstanding PRs