fix and enforce convention on white-spaces, code-indentation and other coding style

[KrisThielemans]

Current code uses a mixture of conventions from different editors etc. We've had some discussion on this on the stir-devel mailing list, see https://sourceforge.net/p/stir/mailman/message/34996792/.

There's a few issues here:

1. how will git handles changes in white-space (as I'm worried about generating tons of conflicts in future branch/PR merges)
2. what style and tool to use
3. how to enforce that future commits use the correct style

Following comments address these.

[KrisThielemans]

@NikEfth has done some more experiments on issue 1 above (how git handles changes in white-space). Here's his email, essentially saying that as long as we make sure that white-spaces get corrected before any merge, we should be fine.

I did the small git experiment: I used the astyle application because it has great reviews and interfaces to 3-4 major IDEs. In addition, it can be setup from an external text file, which we can supply and tell everybody just to use this file when they call the application.

In the style I made, inside functions I delete the empty lines, unless there is some sort of block around, were I create new empty lines to make the block stand out.

Step 1: Format Scanner [.cxx and .h] in a specific style using the astyle application in branch "style2" (history). commit and pushed

Step 2: Format Scanner [cxx and h] with the same style in master.

Step 3: pull request The diff command in git hub interface continued to show differences between the files. Even that they were identical. But no conflicts emerged.

Step 4: In branch "style 1" which has a modified Scanner.cxx with different options and is older from "style 2" I tried to merge the new format from master using:

git merge origin/master

and I had conflicts in the Scanner.cxx.

Step 5: In style 1 I formated the Scanner.cxx as it is in the master and the merge returned: Already up-to-date.

----------- Experiment 2 Now I will repeat the same but in addition to the formatting I will add and remove some code.

Step 1: New branch "code 1"

Step 2: Format with astyle only the master.

Step 3: From arccorrection.cxx I delete a function and add a new one.

Step 4: Merge the formated code form master to the code1 : Failed.

Format the file in code1

Merge : Success

[KrisThielemans]

Issue 2: tool/conventions to use

I believe that a lot of STIR was formatted with emacs default c++-mode settings (no tabs), but this might have become a minor fraction by now.

Nikos used astyle with the following

--style=allman
--indent=spaces=2

--attach-extern-c

--indent-classes
--indent-modifiers
--indent-switches
--indent-cases
--indent-namespaces
--indent-labels
# This could be removed, as well 
--indent-preproc-block
--indent-preproc-define
# Not in Style2 
#--indent-preproc-cond
--indent-col1-comments

--break-blocks=all
--pad-oper
--pad-header

--unpad-paren

--align-pointer=type
--align-reference=type
--delete-empty-lines

Another well-known tool is clang-format. This might not do matlab or Python (not sure).

[KrisThielemans]

Issue 3: git integration

we'd have to prevent new commits messing things up again, while not making a contributor's life too complicated. There's a lot of tools for this, involving git hooks, server-side stuff etc. Advice sorely needed. Possibly @dvolgyes, @casperdcl can help?

Examples used by CMake are at https://gitlab.kitware.com/cmake/cmake. This combines usage of a .clang-format, modification of .git-attributes and some hooks. I've noticed that with this a "git diff" automatically colors white-space errors (but the hooks don't get set automatically). There's also a git plugin for helping with clang-format integration, see for instance https://dx13.co.uk/articles/2015/4/3/Setting-up-git-clang-format.html.

[casperdcl]

I always have a .clang-format, but to avoid contributors from having to have it installed (and also git saving/overwriting files while they're open and modified in an IDE/text editor) I avoid adding a hook. It's up to devs to ensure formatting is correct before a merge.

[casperdcl]

To be explicit, I'd propose:

• (repo) .gitattributes

  [attr]cppfiles  eol=lf  filter=cppfmt
  [attr]cfiles    eol=lf  filter=cppfmt
  [attr]pyfiles   eol=lf  filter=pyfmt
  *.cpp  cppfiles
  *.hpp  cppfiles
  *.h    cfiles
  *.c    cfiles
  *.py   pyfiles

• (repo) .clang-format

  ---
  Language:      Cpp
  BasedOnStyle:  LLVM
  Standard:      Cpp11
  ...

• (repo) .style.yapf

  [style]
  split_before_named_assigns=False

• (local) config

  $ git config --global filter.cppfmt.clean clang-format
  $ git config --global filter.pyfmt.clean yapf
  $ sudo apt-get install clang-format
  $ pip install yapf

Not sure about linting for matlab files.

[casperdcl]

The other advantage of the above is users can easily turn off linting (e.g. if it takes too long):

~/UCL/STIR$ git config --local filter.cppfmt.clean cat

And of course if filter.cppfmt is undefined (never configured because you're a casual end user) then the filter is silently ignored.

[dvolgyes]

Well, there are a lot of technical issues here.

First, there is no way to actually prevent a git commit. Period. It happens on the user's side, he/she can do anything. (But you can help the users to follow the guides.)

This is the reason why github has no pre-commit hook. The commit has already happened on the users computer. If he/she has rights to update the repository, there is no way to prevent this.

However, you can enforce rules through the pull requests: you don't have to merge, if you are not satisfied with the PR.

There are several bots, like the "pep8speaks" which tests the PR, ( https://pep8speaks.com/ ) and comments it. I am not aware any C++ version, but it might exist. You can take a look at Github's market place and the "works-with" directory. https://github.com/marketplace https://github.com/works-with

The usual way to enforce special rules, like style, is through continuous integration.

• make code formatting (lint, static analyzer, etc.) part of the tests
• if the PR does not conform with the rules, then the build process will fail
• Travis/AppVeyor can upload artifacts to a site, so if the log isn't detailed enough, then you could check this file for further details

This has the advantage that any tool can be used, even quite specialized one. For instance, the documentation could be spell-checked, and if the spell-check fails, then the build fails. (But I wouldn't go that far.)

So what could be incorporated?

• astyle http://astyle.sourceforge.net/
• uncrustify http://uncrustify.sourceforge.net/

I liked astyle more, but uncrustify seems to be more maintained. (It has Travis builds, recent commits, etc.)

Theoretically, anything could work. The rules could be enforced with local pre-commit hooks, or with a simple make file, like "make format" before commit.

But I would argue for a server-side protection. And the only way I see this feasible, is through pull requests. If there is a "pep8speaks"-like c++ bot, then it could work. Otherwise I would argue for a "make test-format" option, where astyle/etc. could format the code, git detect if there is a modification (git status), and if there was one, then the code did not pass the style check.

Astyle/uncrustify or anything else: there are dozens of similar apps, starting with clang tools, but, I haven't seen any integrated one for github.

In the end, it depends on the usability, etc. Enforcing clang code formatter on windows or Mac could be quite challenging. And the same for installing a working pip version. So I would not modify the git config files. I think the initial cost contributing to STIR is already high enough.

Enforcing through Travis is easier, but it has the disadvantage that the actual debugging (figuring out what is wrong with the commit) could be more challenging. (It might be hard to find in the log file.)

That's my opinion. (Besides that: I do not really care. I try to keep the current code style of the file, and if I just want to understand the code, then I enforce my own rules, make a patch, and reformat the patch in the old style.)

astyle/emacs/vi/eclipse/etc.: It would be nice to choose a universally applicable style but it is not always possible. E.g. tools differ in tiny things, like: do they count the line length from the first character or the first non-whitespace character? (The answer depends on the tool.)

[casperdcl]

@dvolgyes I think @KrisThielemans's point was that we want liniting but don't want the casual user to be put off because of it. So my suggestion was to:

1. Add linting config files to the repo (.gitattributes, .clang-format, .style.yapf)
2. Tell maintainers to configure local hooks and install linters
3. Protect master branch so only said maintainers can push to it
4. As you suggest add checks to .travis.yml (where it's easy to apt-get or pip install things, and parse clang-format --output-replacements-xml or python -m flake8 --exit-zero without doing any inplace-lint-and-check-git-status-for-modifications)

[KrisThielemans]

I have had another read at this. So this looks mostly ok to me.

there's 2 things I'd like to get some feedback on:

1. as we're not imposing this on the (casual) contributor, how do we handle PRs? I guess, the contributor creates the PR, we run the tool to clean it all up, we commit that to the PR, and tell the contributor to check.

2. we need guidelines for (not so casual) contributors on editor settings. this might be hard, but we should cover the most important ones. I guess this point will determine the final style/tool that we use.

Anyone wants to create a PR for this?

[dvolgyes]

I have no useful idea for cleaning up PRs, but there is one for the enforcement part. There is a tool called precommit-hooks. (See pre-commit.com and https://github.com/pre-commit/pre-commit-hooks ). This is a framework for handling git pre-commit hooks, and it is extendable with plugins which are easy to write. The configuration file is stored in the .pre-commit-config.yaml, and it is ignored, unless the developer actively enables it with: "pre-commit install" After that every commit is checked with the hooks, but they can be disabled for false positives. The hook even can re-write the code, e.g. with astyle. This aborts the commit because it is a bad idea to check in code without taking a look, but with repeated attempt it will accept. (because second time the code does not change).

I was experimenting with the framework, and it is quite OK, e.g. I could write a plugin in 30 minutes which checks image/video/audio file integrity, and rejects corrupted files.

First of all, writing plugins is easy, secondly, it does not do anything, unless enabled.

It is pretty well designed, multi-platform, and I did not find anything at least similarly capable alternatives. (Keep in mind, this is a meta-problem, you still have to decide about code style, etc., but this tool could enforce the decisions.)

[dvolgyes]

By the way, here is a longer list of hooks. I did not find astyle hook, but it shouldn't be too complicated. https://pre-commit.com/hooks.html

[ashgillman]

Perhaps a format could be decided upon for new additions to begin with, with no formal enforcement (avoiding discussions on existing files, merges/PRs/forks, enforcement).

So far suggested have been astyle, clang-format and uncrustify. flake8 is pretty universal for Python (if you're interested, look at black, but it is too much for here).

[KrisThielemans]

EditorConfig seems pretty good...

[ashgillman]

EditorConfig doesn't have quite enough options.

E.g.,

function(a_long_arg,
         b_long_arg;
function(
  a_long_arg, b_long_arg);

curly_brackets {
  like_this;
}
curly_brackets
{
  like_that;
}

EDIT: yet https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#ideas-for-domain-specific-properties

[KrisThielemans]

I've had a brief look at this. It seems that support for clang-format is now pretty good, with plug-ins even available for Visual Studio (and VS Code).

My current understanding is that these plug-ins allow you to format a section of code, but they don't set editor settings from the .clang-format. Do correct me if I'm wrong. But if not, I think it needs to be paired with some config files for editors that "we" use, and even a EditorConfig file for minimal settings (no tabs, indentation-size, etc). And of course recommendations for developers.

It all sounds daunting again, but we should at least make a baby-step soon. I like @casperdcl's suggestion. Next thing would be to come up with a .clangformat that I'm happy with! (as I said, I'd like it to be similar to default emacs C++ settings, as that's what a sizeable part of STIR uses)

[KrisThielemans]

I've played around a bit with clang-format. There doesn't seem to be an easy setting that changes hardly any files (as @NikEfth already long ago told), as there's just no consistent style in STIR in use of course. However, most of the indentation follows the GNU style. I seem to be fairly happy with the following changes w.r.t. GNU (although of course, making any changes might be a bad idea), see the clang-format doc

AlwaysBreakAfterReturnType: TopLevelDefinitions
AlwaysBreakTemplateDeclarations: true
  SplitEmptyFunction: false
ColumnLimit:     130
IndentPPDirectives: AfterHash
PointerAlignment: Left
SortIncludes:    false
SortUsingDeclarations: false
SpaceBeforeParens: ControlStatements

Some motivation:

• I don't like automatic reordering of include files. Who knows what that will do.
• sorting using using declarations would be fine but introduces extra diffs
• 79 characters on a line is a bit out-dated.

There's still a large amount of changes moving

int
A::
func()

(which is a FSF recommendation) to

int
A::func()

but I didn't find a setting for this, and we're not consistent with this style anyway.

Here's the full .clang-format that I ended-up with (I had to rename it to put it here)

If anyone has any strong opinions, or finds a setting that changes less things (while still doing something!), feel free to chip-in!

[dvolgyes]

If clang-format, or any other formatter is used, then i would strongly suggest introducing a pre-commit hook too, e.g. for clang-format: https://github.com/doublify/pre-commit-clang-format

Pre-commit is awesome, and i highly recommend it for any and every project. :) https://pre-commit.com/

There are other hooks too, not just formatting, e.g. cpp-check could check for new code issues, but also there are hooks for malformed python code, or accidental files, like object files, etc. e.g. https://github.com/pocc/pre-commit-hooks

(For me, docker, CI/CD and pre-commit are the top 3 developer tools of the past decade. I am not sure in the order.)

[KrisThielemans]

thanks @dvolgyes looks interesting! If you happen to be able to suggest a small .pre-commit-hooks.yml, it'd save me some time :-) I'm sure we might have to do this somewhat gradually.

[KrisThielemans]

Note: the only way I found to download clang-format (as executable) for Windows is to install LLVM. A bit overkill unfortunately. I guess it gives you a good compiler...

[ashgillman]

Apparently you can do it with Node.js' npm:

Note that on Windows, using the Node.js option seems to be the simplest solution for obtaining just clang-format and git-clang-format (without installing MinGW).

https://superuser.com/a/1505297

[KrisThielemans]

reopened as we provided the infrastructure but haven't actually cleaned-up the source yet, The intention is to first merge 1 or 2 major PRs.

However, developers should already configure their editor to follow the conventions in our .clang-format. PR with doc coming

[KrisThielemans]

see #970