Applying clang-format to the entire repository?

[ksuther]

Is this project open to using clang-format to format entire repository? We're working within our own fork and the inconsistent formatting style is an impediment to easily merging, rebasing, and eventually upstreaming changes.

There's already a .clang-format file in the repo that doesn't appear to be used (https://github.com/gnustep/libs-base/blob/master/.clang-format). If we can get a format that everyone agrees on then we can add pre-commit and tie it in to GitHub Actions to keep the style in place.

We're willing to do the work and submit a PR, but we don't want to go down that road this if it won't be accepted.

[hmelder]

Hey,

most of the code is hand-formatted and does not follow the clang-format configuration. Some formatting features, such as spaces after ObjC Argument Types, are not supported by clang-format.

However, I am also using it for my PRs but constantly enabling and disabling auto-formatting for existing code is quite tedious.

[gcasa]

@ksuther Hey Kent, I'm @gcasa the lead developer of the project. GNUstep, of course, predates clang-format, thus we have mainly used manually formatting over the years. I personally use the format that emacs imposes, but that's just me. Currently, we are close to a release date. After that we can consider what you're discussing.

Any format must be consistent with our existing coding standards. They are here.

Each of the packages has its own maintainer as well and they must also have input into any format that is agreed upon and any procedural changes this incurs. @fredkiefer maintains libs-gui and @rfm maintains libs-base. I maintain apps-gorm, libs-xcode, and am considered the "default" maintainer for any packages within the project that doesn't have a current maintainer.

Other maintainers include @davidchisnall for libobjc2, @rmottola for apps-gworkspace and apps-projectcenter.

I also feel like other major developers on the project should be included on this: @hmelder , @triplef , @qmfrederik. There are others that I may have left out of this list, but their opinions are no less important on this matter.

While formatting might not be a huge deal with respect to functionality it is important for maintainability and traceability. The way I see it we have two choices:

1. Do it peace-meal. On a case by case basis.
2. Reformat the entire repository in one shot.

Each are a ton of work. It's something to consider when thinking about how to go about this.

GC

[rfm]

As Hugo says, it appears that clang-format is not currently capably of producing nicely formatted files, with the most significant issue being that it loses the whitespace used to make method names easy to read.

But ... if we could get clang-format improved a bit, my feeling is that it would be good enough so that the benefits of automated formatting would outweigh the disadvantages (making it worth change coding style rules a little).

[davidchisnall]

However, I am also using it for my PRs but constantly enabling and disabling auto-formatting for existing code is quite tedious.

There's a clang-format-diff.py script included with clang-format that could probably save you some time. It applies clang-format changes only to the lines that have been touched in a diff.

[rmottola]

spaces after methods are tedious :)

We should be sure that clang-format outputs the same format that emacs uses with obj-c, to avoid tedious ping-pong of formatting.

I hate big-reformats, they ruin history reading... it was done several times in other projects (e.g. mozilla) and it makes things a pain. But it is good to format new things or reformat "touched code" so that piecewise things align. not pefect, but that's it for 20-30 years old code. I thus like @davidchisnall suggestion to run it on the diff lines.

[ksuther]

Thanks for the input. I agree that code formatting tends to be tedious no matter what, so I realize I'm opening a can of worms here :)

Creating an ignore file such as .git-blame-ignore-revs (see https://moxio.com/blog/ignoring-bulk-change-commits-with-git-blame/) can help prevent ruining blames.

It sounds like having a clang-format style that everyone can agree on is the main blocker to doing anything. Is no space after method colons the only thing in clang-format that's missing or are there others? I've submitted patches to clang-format many years ago, and can take a swing at adding that.

[hmelder]

Is no space after method colons the only thing in clang-format that's missing or are there others?

I think it is the main blocker. You can check the clang-format output (GNU Preset + the custom Objective-C Settings) against existing code and the coding guideline.

[qmfrederik]

Perhaps uncrustify is another option for enforcing (very basic) code formatting standards. I ran a quick experiment and came relatively close using this configuration:

output_tab_size = 8
indent_columns = 2
indent_brace = 2
align_with_tabs = true
align_on_tabstop = true
align_var_def_span = 8
align_var_def_star_style = 1
align_var_def_amp_style = 1
sp_after_ptr_star = remove

In case anyone is interested, all options are defined here: https://github.com/uncrustify/uncrustify/blob/master/src/options.h

[rfm]

I don't use clang-format, so I don't know exactly what it does right and what it does wrong, but way back when I last looked into automated reformatting one of the problems I found was with indentation of long method calls.
For a short call everything fits on one line, with a space after each colon.
In an ideal world if a line is too long the coding standard is that the method call is broken up with the colons at the end of each part of the method name stacked above each other (I find that a great visual reference point), but sometimes the individual parameter of the method call are too long to fit on a line if the colons are lined up. So in this case the standard is that each part of the method name is indented two spaces from the start of the method invocation.
I never found anything that got that right, but I think it woudl be good enough if a tool didn't alter existing correct formatting.

[rmottola]

Another issue is respecting indentation with macros. e.g
NS_DURING NS_HANDLER NS_ENDHANDLER

emacs gets it wrong, imo, so I tweak it manually. Careless reformatting causes issues.
Similar is with long method name indenting...

[rfm]

Looking at uncrustify, I'm mot sure how good it is but it sounds like the authors are fairly actively seeking objc improvements, so there may be an opportunity to make sure it has an option to format in gnustep style. That would be good.

[davidchisnall]

In an ideal world if a line is too long the coding standard is that the method call is broken up with the colons at the end of each part of the method name stacked above each other (I find that a great visual reference point), but sometimes the individual parameter of the method call are too long to fit on a line if the colons are lined up. So in this case the standard is that each part of the method name is indented two spaces from the start of the method invocation.

clang-format does this right. Not surprisingly, given that it's the thing that XCode uses and the Objective-C bits were mostly written by Apple folks. The only bugs I've seen have been related to the tabs-for-indentation-spaces-for-alignment mode, which GNUstep doesn't do.

Another issue is respecting indentation with macros. e.g
NS_DURING NS_HANDLER NS_ENDHANDLER

I believe there's some support in clang-format for defining macros that should be treated as { and }. That said: Does GNUstep still support any compilers that don't do native Objective-C exceptions? I think they were added in GCC 3.x.

[rfm]

It's not perfect, but here is an uncrustify config file which seems close.
objc.txt