Update coding guidelines to match reality

[bernhardmgruber]

The current alpaka coding guidelines may be out of date. For example, during a review @mehmetyusufoglu rightfully pointed out, that the use of non-sized built-in types like int or unsigned is forbidden in alpaka.

I think this is nonsensical.

I propose the following changes:

• Remove all rules that are handled by clang-format.
• Do not recommend variable initialization using paranthesis: int i(0); should not be the default and even discouraged IMO. I think we also regularly break the rule against int i = 0;. I write lots code like that, and people seemed fine with it? Encouraging parenthesis, int i{0};, is fine.
• Drop suggesting comment blocks before type or function definitions. It only adds noise.
• Drop the recommendation to put comments next to includes telling which feature is included from that header. This is unmaintainable, does not help IMO, and will go away in C++23 anyway.
• In the future, we should also remove all rules that clang-tidy is taking care of, e.g. identifier namings.

[fwyzard]

• Remove all rules that are handled by clang-format.

👍🏻

• Do not recommend variable initialization using paranthesis: int i(0); should not be the default and even discouraged IMO. I think we also regularly break the rule against int i = 0;. I write lots code like that, and people seemed fine with it? Encouraging parenthesis, int i{0};, is fine.

👍🏻 (I find int i(0); pretty horrible)

• Drop suggesting comment blocks before type or function definitions. It only adds noise.

I'm not sure about this... in general, a comment explaining what a function does is pretty useful to have.

Of course, sometimes they are just trivial, indeed.

• Drop the recommendation to put comments next to includes telling which feature is included from that header. This is unmaintainable, does not help IMO, and will go away in C++23 anyway.

👍🏻

• In the future, we should also remove all rules that clang-tidy is taking care of, e.g. identifier namings.

I'm interested in this, because I tried using this specific functionality in the past and it failed horribly...

[bernhardmgruber]

• Drop suggesting comment blocks before type or function definitions. It only adds noise.

I'm not sure about this... in general, a comment explaining what a function does is pretty useful to have.

Of course, sometimes they are just trivial, indeed.

I am referring to a mandatory comment of:

//#############################################################################

before each type, and a comment of:

//-----------------------------------------------------------------------------

before each function. Any meaningful documentation is of course fine!

• In the future, we should also remove all rules that clang-tidy is taking care of, e.g. identifier namings.

I'm interested in this, because I tried using this specific functionality in the past and it failed horribly...

I am enforcing this for years for LLAMA as part of the clang-tidy CI. Someone would just need to add a clang-tidy CI job to alpaka. I failed in the past, because the alpaka build scripts were too annoying to adapt.

[fwyzard]

I am enforcing this for years for LLAMA as part of the clang-tidy CI. Someone would just need to add a clang-tidy CI job to alpaka. I failed in the past, because the alpaka build scripts were too annoying to adapt.

Ah... so, does clang-tidy just check for it, or does it also apply the fixes automatically ?

[mehmetyusufoglu]

I strongly agree with the proposal.

[psychocoderHPC]

• Drop suggesting comment blocks before type or function definitions. It only adds noise.

I'm not sure about this... in general, a comment explaining what a function does is pretty useful to have. Of course, sometimes they are just trivial, indeed.

I am referring to a mandatory comment of:

//#############################################################################

before each type, and a comment of:

//-----------------------------------------------------------------------------

before each function. Any meaningful documentation is of course fine!

• In the future, we should also remove all rules that clang-tidy is taking care of, e.g. identifier namings.

I'm interested in this, because I tried using this specific functionality in the past and it failed horribly...

I am enforcing this for years for LLAMA as part of the clang-tidy CI. Someone would just need to add a clang-tidy CI job to alpaka. I failed in the past, because the alpaka build scripts were too annoying to adapt.

I agree that this is horrible and does not reflect how we write code.

offline discussed: IMO the best base to discuss these changes is to open a PR. Then there is no need to explain which rules you would change or remove. If we only discuss is in an issue and nobody reflects the discussion in a PR it is wasted time.

[bernhardmgruber]

I am enforcing this for years for LLAMA as part of the clang-tidy CI. Someone would just need to add a clang-tidy CI job to alpaka. I failed in the past, because the alpaka build scripts were too annoying to adapt.

Ah... so, does clang-tidy just check for it, or does it also apply the fixes automatically ?

clang-tidy can apply fixes automatically, including changing the coding style. I have not added this to a CI job, because I still frequently end up adding exceptions and NOLINT comments when I disagree with clang-tidy. But in principle, the tool is capable of automation.

[fwyzard]

Yes, we use clang-tidy with automatic fixes regularly in the CMS software.

What I meant is that I tried to use the rules for member variable names in the past, but the automatic fixes were not working for it.

[bernhardmgruber]

What I meant is that I tried to use the rules for member variable names in the past, but the automatic fixes were not working for it.

When we first settled on the style rules, I ran clang-tidy myself on LLAMA and applied the changes. It didn't catch everything, but probably a good 95%.