Project

General

Profile

Task #2107

Change to clang-format

Added by Roland Schulz over 2 years ago. Updated 12 days ago.

Status:
Accepted
Priority:
High
Assignee:
-
Category:
-
Target version:
-
Difficulty:
uncategorized
Close

Description

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:
https://gerrit.gromacs.org/#/c/6307
https://gerrit.gromacs.org/#/c/6414
https://gerrit.gromacs.org/#/c/6415
https://gerrit.gromacs.org/#/c/6416
https://gerrit.gromacs.org/#/c/6417

As a result multiple bugs were identified as affecting us in an important way (many newly filed by us):
https://llvm.org/bugs/show_bug.cgi?id=27353
https://llvm.org/bugs/show_bug.cgi?id=31648
https://llvm.org/bugs/show_bug.cgi?id=31649
https://llvm.org/bugs/show_bug.cgi?id=17362
https://llvm.org/bugs/show_bug.cgi?id=22827
https://llvm.org/bugs/show_bug.cgi?id=31664
https://llvm.org/bugs/show_bug.cgi?id=31681

This could be revisited when the important of those have been resolved.

History

#1 Updated by Roland Schulz 11 months 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 11 months 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.

#3 Updated by Szilárd Páll 11 months ago

Would patching clang-format be feasible (and sustainable) effort for seemingly minor things (like alignment, though admittedly I've not checked whether that's indeed a minor change) in order to minimize the footprint of a full-repo reformat?

#4 Updated by Roland Schulz 11 months 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 11 months 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.

#6 Updated by Berk Hess 6 months ago

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?

#7 Updated by Berk Hess 6 months ago

  • Status changed from New to Accepted
  • Priority changed from Low to High

#8 Updated by Eric Irrgang 12 days 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 grep of uncrustify in the source tree...

Also available in: Atom PDF