Change to clang-format
It might be beneficial to switch to clang-format because 1) it has better modern C++ support and 2) it is designed fully automatic formatting which allows fully automatic refactoring.
The most recent attempt of doing the switch was in Jan 2017. The gerrit issues can be found at:
As a result multiple bugs were identified as affecting us in an important way (many newly filed by us):
This could be revisited when the important of those have been resolved.
Add initial .clang-format file.
This change separates out the introduction of the clang-format
configuration from the adoption of clang-format as the alternative to
Discussion on the contents of the config file have taken place at
#1 Updated by Roland Schulz about 1 year ago
Besides random bugs, our version of uncrustify misformats braced initializer lists and ">>" in templates. It might help to upgrade to a newer version. But this won't address all issues. In particular I think we would benefit from having a maximum line length but uncrustify isn't able to split strings automatically. And most of our insanely long lines are because of long strings.
I personally thing that the advantages of clang-format outweigh the disadvantages by far. And I even think that's true with pretty much any clang-format configuration. It would be nice to know what people consider blocking issues in clang-format. If there are only a few relatively easy to fix blocking issues I would look into fixing those in clang-format.
#2 Updated by Roland Schulz about 1 year ago
This was discussed at today's call.
An extra issue was raised: We often align with more spaces than necessary. clang-format doesn't support this and would likely be hard to upstream this functionality because it seems to be not inline with its design goals. This has impact on the size of the initial reformatting change. It also means that adding/removing lines can trigger white-space changes on neighboring lines more easily.
As possible solution too the large number of changes in the original reformat, only reformatting lines as they get changed, was mentioned. This is possible with clang-format-diff. This might be sightly inconsistency between new lines formatted with clang-format and old lines. It also might mean that alignment is not always consistent between modified and unmodified lines.
#4 Updated by Roland Schulz about 1 year ago
We could patch clang-format. But I'm less exited about making changes which we don't try to upstream. Both because this requires everyone to install a modified version of clang-format (and that is harder to distribute and build than uncrustify) and because it requires continued effort to rebase those changes if we want to update clang-format semi regularly.
#5 Updated by Mark Abraham about 1 year ago
I'm 100% in favour of vanilla clang-format (we considered it some years ago and concluded that we really wanted something coming in version 3.8, but that's fine now). Getting new devs on board easily is much more valuable than the cost to existing devs of bumping the indentation one time (if we have to). Rebasing over that patch is a simple matter of accepting the changed code, and then applying the formatter.
We should switch to clang-format as soon as we can.
There didn't seem to be strong objections to the switch and the formatting suggestion that Roland uploaded. IIRC I had the strongest objections, but I can live with the things I don't like so much, in particular spaces around * and /.
I just looked at the formatting again and there I commented on some minor inconsistencies. I also noticed several alternatives from pointer * alignment Roland mentioned. Is there a clear preference for one of these?
#8 Updated by Eric Irrgang 2 months ago
Some additional tasks:
- [ ] update admin/uncrustify.sh (or its dependents)
- [ ] update reformat_all.sh
- [ ] update docs/dev-manual/uncrustify.rst
- [ ] update docs/release-notes/2020/major/tools.rst
- [ ] update jenkins.rst (and presumably jenkins)
I won't bother attaching a complete
uncrustify in the source tree...
#9 Updated by Eric Irrgang about 2 months ago
I can't tell if https://gerrit.gromacs.org/c/gromacs/+/6307 is moving closer to acceptance, but if it is accepted in its current state, the effect will be that subsequent Gerrit changes will require reformatting for new uncrustify rules, and clang-format will still not be in use. It seems like maybe a different approach is needed.
1. Add a Jenkins test (or update uncrustify.py) for "cpp-formatting" that will run clang-format for files in the current commit that have the "clang-format" filter set in their git attributes.
2. Accept the .clang-format file in https://gerrit.gromacs.org/c/gromacs/+/6307 by itself.
3. Replace the "uncrustify" git attribute with the "clang-format" attribute in a slow-moving GROMACS module, reformat, and commit, making minor tweaks to the clang-format file to minimize diffs. Repeat this step two or three times if necessary to reach a .clang-format that is reasonable for the GROMACS 2020 release.
4. Encourage developers to migrate and reformat the modules they are actively working on and to rebase any outstanding Gerrit changes on these reformatting changes.
5. Before splitting release-2020 from master, replace all remaining "uncrustify" attributes with "clang-format" attributes and reformat.
6. Where it seems important and (3) did not seem adequate, allow per-directory .clang-format files to evolve during the GROMACS 2021 development cycle and set a task to consolidate and reformat before the 2021-infrastructure-stable milestone.
#11 Updated by Eric Irrgang about 2 months ago
On the dev telco, Roland mentioned some sort of convenient stuff that could be done with git and clang-format to make things easier. I'm not sure I found exactly what he was talking about, but I found that
clang-format installs a script called
git-clang-format that can then be invoked by a plugin mechanism I didn't know about, with
git clang-format. There is no man page, so
git clang-format --help and
git help clang-format don't find anything, but
git clang-format -h give usage information. Various examples of incorporating this command into a git pre-commit hook appear with a quick Google search.
#12 Updated by Roland Schulz about 2 months ago
I was referring to merge.renormalize. See https://git-scm.com/docs/merge-strategies#Documentation/merge-strategies.txt-renormalize and https://git-scm.com/docs/gitattributes#_merging_branches_with_differing_checkin_checkout_attributes .