Project

General

Profile

Task #2107

Change to clang-format

Added by Roland Schulz over 2 years ago. Updated about 2 months 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.


Related issues

Blocked by GROMACS - Bug #3053: GPU_FUNC_TERM and GPU_FUNC_TERM_WITH_RETURN usage confuses clang-formatClosed

Associated revisions

Revision 973c5eee (diff)
Added by Eric Irrgang about 1 month ago

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
uncrustify.

Discussion on the contents of the config file have taken place at
https://gerrit.gromacs.org/c/gromacs/+/6307

Refs #2107

Change-Id: I2221486ebd24147799338a3be7ca462e9c8e761a

History

#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.

#3 Updated by Szilárd Páll about 1 year 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 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.

#6 Updated by Berk Hess 8 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 8 months ago

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

#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 grep of 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.

Proposal

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.

#10 Updated by Eric Irrgang about 2 months ago

  • Blocked by Bug #3053: GPU_FUNC_TERM and GPU_FUNC_TERM_WITH_RETURN usage confuses clang-format added

#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.

Also available in: Atom PDF