Project

General

Profile

Task #664

Update code formatting to follow coding conventions

Added by Mark Abraham almost 10 years ago. Updated almost 8 years ago.

Status:
Closed
Priority:
Low
Assignee:
-
Category:
-
Target version:
-
Difficulty:
uncategorized
Close

Description

Coding conventions for new GROMACS code exist (http://www.gromacs.org/Developer_Zone/Programming_Guide), however much of the old code does not (yet) follow it.

1) Should there be a wholesale re-formatting some time? Presumably some tool can automate this.

2) Where the existing formatting inhibits understanding, it should be changed to the above regardless.


Related issues

Is duplicate of GROMACS - Task #845: Format white space correctlyClosed11/25/2011

Associated revisions

Revision 2bb262b4 (diff)
Added by Mark Abraham almost 10 years ago

Made src/kernel/repl_ex.c legible

Some of the formatting was extremely awkward to read, so I
re-formatted the whole file with emacs using the formatting line, and
added braces where consistent with the coding conventions.

IssueID #664 point 2.

History

#1 Updated by Mark Abraham almost 10 years ago

Note that git blame -w can be used to ignore whitespace changes when tracking code back through time, and that other diff-using git tools (e.g. git log and git diff) can generally do likewise. This isn't perfect, because a commit that turns

if (bCondition) {
    do_something();
}

into
if (bCondition)
{
    do_something();
}

will still show with git blame -w as the last commit to touch the line with the conditional.

Similarly, git log -p --color-words -w is also less than ideal.

#2 Updated by Teemu Murtola almost 10 years ago

indent (a Unix command-line tool) could be used for bulk formatting and can probably be tuned to produce code that's reasonably close to the coding standard, but I'm not sure whether that's a good idea. The main problem is that although the git diff tools can handle reformatting relatively well as Mark pointed out, the merge tools probably can't. If someone wants to merge a branch that has reformatted many files that have also been modified in the target branch, he is more or less forced to apply the exactly same formatting in the target branch manually, otherwise the likely result will be a conflict in the whole file, which can be very annoying to resolve...

I think a reasonable course of action is to reformat source files if/when it is planned that they will be substantially modified, and preferably at a time when they have not been significantly modified in other branches. One thing that one should never ever do is to apply reformatting in a commit that also does any functional changes. Otherwise, problems in merging and tracking changes will be even worse.

#3 Updated by Mark Abraham almost 10 years ago

Teemu Murtola wrote:

indent (a Unix command-line tool) could be used for bulk formatting and can probably be tuned to produce code that's reasonably close to the coding standard, but I'm not sure whether that's a good idea. The main problem is that although the git diff tools can handle reformatting relatively well as Mark pointed out, the merge tools probably can't. If someone wants to merge a branch that has reformatted many files that have also been modified in the target branch, he is more or less forced to apply the exactly same formatting in the target branch manually, otherwise the likely result will be a conflict in the whole file, which can be very annoying to resolve...

I think a reasonable course of action is to reformat source files if/when it is planned that they will be substantially modified, and preferably at a time when they have not been significantly modified in other branches. One thing that one should never ever do is to apply reformatting in a commit that also does any functional changes. Otherwise, problems in merging and tracking changes will be even worse.

Indeed. If the reformatting commit has no code-change content, then merging reformatted code with a branch that has changed the content (while in the old format) can be a simple matter of taking the content-modified file and applying a formatting filter to it, rather than resolving a large bunch of merge failures by hand. As I did for 2bb262b4cc2, git diff -w (perhaps with --color-words) can then be used to verify that the merge commit consists only of re-formatting.

#4 Updated by Rossen Apostolov about 9 years ago

  • Assignee deleted (Erik Lindahl)

#5 Updated by Teemu Murtola over 8 years ago

  • Category deleted (mdrun)
  • Target version set to 4.6

Marked target version as 4.6, since the (mostly) duplicate issue #845 has that as well.

#6 Updated by Erik Lindahl almost 8 years ago

  • Target version changed from 4.6 to 5.0

Changing target to 5.0, since we're not going to do large overhauls of whitespace and/or formatting in the very last minute before 4.6 the release.

#7 Updated by Teemu Murtola almost 8 years ago

  • Status changed from New to Closed
  • Target version deleted (5.0)

Closed as a duplicate to avoid iterating the target version for both back and forth...

Also available in: Atom PDF