Project

General

Profile

Task #2107

Change to clang-format

Added by Roland Schulz almost 3 years ago. Updated 4 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 3 months 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

Revision 187a2c00 (diff)
Added by Teemu Murtola 18 days ago

Update clang-format config

Provides minor changes to the clang-format configuration, and adds
ability to use reformat_all.sh to change the source tree formatting
to be clang-format conform.

Some attempt has been made to match changes in uncrustify.cfg to be
closer to clang-config but not much work has been put into this direction
since there are about 450K lines are different between the two formatting
methods.

You can run
admin/reformat_all.sh
to reformat everything with uncrustify, and
admin/reformat_all.sh clang-format
to format the same files with clang-format (the executable can be
specified with CLANG_FORMAT).

Refs #2107

Change-Id: Ia1880f5670714c497f683be10a9e72b505ca6c5a

Revision 6d0f1b6e (diff)
Added by Paul Bauer 18 days ago

Add build for clang-format

Adds build for running clang-format in Jenkins, and sets the correct
filters for uncrustify not to apply changes on files now checked by
clang-format instead.

CHanges default to use clang-format for reformat-all.sh

Refs #2107

Change-Id: I62c1f10be5fcda495fea4dbe5a23fa5f006d52a0

History

#1 Updated by Roland Schulz over 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 over 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 over 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 over 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 over 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 10 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 10 months ago

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

#8 Updated by Eric Irrgang 4 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 4 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 4 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 4 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