define the C++ coding style

[lynxlynxlynx]

Previous discussion here: #158

Basically Tom's original proposal has been lost and we have to start from scratch (or someone could ask him, if he has a copy, of course). The goal is to reduce ambiguity, eventually inconsistency and enable the use of patch formatters to integrate their checking with CI (plus potentially local git hooks).

http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

Check if there is a style-deducer for some format, something that would read the code and assert a style format. Otherwise it will probably have to be trial and error until a format with minimum style violations is found.

QGIS has an overly complicated script that could be trimmed down and reused (runs astyle as a pre-commit hook) https://github.com/qgis/QGIS/blob/master/scripts/prepare-commit.sh

gemrb/docs/en/CodingStyle.txt has just our comprehensive "#include" ordering rules. The core style ideas are noted in CONTRIBUTING.

notes: uppercase the literal suffix 0.0f -> 0.0F (avoids confusion with l, 1 and L)

another option: https://github.com/sk-/git-lint

OBS example we could use at least for PRs: https://github.com/obsproject/obs-studio/blob/master/.github/workflows/clang-format.yml

[lynxlynxlynx]

Same goes for python; now that we're part of github actions beta, this pep8 checker needs to be evaluated: https://github.com/marketplace/pep-8-speaks

Who knows how our current style differs and if the checker doesn't choke on our c++ modules (eg. snake-food does). Sonar's pylint run can be configured with a pylint settings file, which means we can set the parameters for the "basic checker" to properly check our names.

This looks useful: https://pre-commit.com/ (also has hooks for clang-format and cpplint).

[lynxlynxlynx]

Github actions also has cpp-lint, clang-tidy and pyton-lint support.

[lynxlynxlynx]

As an interim partial solution we now enforce a few of our whitespace requirements for python in pull requests: https://github.com/gemrb/gemrb/blob/master/.github/workflows/style.yml

Ideally we'd do all the checking in a pre-receive hook, so pushes would verbosely fail if non-compliant. And no setup would be required on the user side, since that runs on the server. Unfortunately github does not support this hook, so only local ones like pre-commit are game if we want to maintain clean history. That means extra deps for developers, but we can make them suggested only and also rely on pull request checkers for non-team changes.

• write the hook (succeeds if the checkers are not present)
• install the pre-commit hook via a cmake target included in the main build target (once/idempotent)
• write the docs, warmly suggest installing the chosen checkers

[kmfrick]

Black for Python might be interesting to take into consideration. Concerning C++, a Clang-format post-receive hook that automatically formats the code (or a pre-receive hook to reject the commit, depending on how we see it) can be a good option like Jaka said.

[lynxlynxlynx]

suy pointed me to https://github.com/johnmcfarlane/unformat, which I'll leave running for a day or so, if it won't terminate itself.

[bradallred]

I've got to say I'm not a fan of the choice to put the * or & next to the variable name instead of the type. As a human I read int* ptr to mean a pointer to an integer named 'ptr' where int *ptr for some reason take cocking my head to read 🤷

I'm inclined to cite Stroustrup himself on this. I've never worked on a C++ project where we do the opposite either.

[lynxlynxlynx]

It's a matter of style, so it can quickly turn into bikeshedding. I prefer this particular alignment for two reasons, but what matters is that this is the most commonly used one in the project, so choosing it is the pragmatic option.

This was an arduous process, but I have much progress to report. That mutator script did an ok job, but far from optimal. I went through the list of options and tweaked further to get the amount of differences down and add rules that were missing (eg. for our include ordering). Compatible with clang10, but there are some commented out directives for clang12, in case it will become useful. Still, it doesn't support everything MISRA standards warn about, but better this than nothing. clang-tidy adds some of our stuff on top, so perhaps that will be doable later. Most notably clang-format has no rules for naming style and case.

What remains:

• [ ] we could pick a different IndentPPDirectives style, since we now mix them; currently set to None, but the other two look better to me; it only affects nested directives
• [ ] commit the .clang-format file
• [ ] pre-commit hook
  • [ ] add a cmake custom command
  • [ ] also for python
• [ ] gh workflow step for PRs
• [ ] test IDE integrations
• [ ] document it all in CONTRIBUTING
• [ ] obviate the tiny CodingStyle.txt

[bradallred]

I guess I'm just shocked to hear its the most common style? Its clearly not in the areas I focus on ;)

I'm not going to start an argument over it, but I don't know why picking a style based on what I presume to be the oldest code that is already in need of clean up due to other style violations would be the pragmatic choice 🤷. I would think deciding based on the preferences of the humans involved would take precedence. We live in the age of scripts and linters so there shouldn't be a cost associated with piking one style over another.

I hope we can at least agree to delay any automated cleanup until after subviews is merged for the sake of sanity.

[lynxlynxlynx]

There won't be any automated cleanup, the idea is to gradually converge and just have a standard going forward. Helps keeping praise/annotate/blame output easier to use.

I've been using this style since I can remember, but I can't remember what it was based on. Likely just the prevalent style. It makes more sense to me and I can explain that, but I don't want to force the issue. Unfortunately the way clang-format works doesn't allow for ignoring specific rules, so we either have to make a choice or pick some other formatter like astyle (doesn't require affecting this placement).

[lynxlynxlynx]

Actually git inherited some stuff from hyperblame, but it still doesn't have a default file for the blame ref exclusion list ... so the plan remains the same. If that ever changes, I wouldn't be opposed to an automatic reformat.

EDIT: just like with the hook addition, we can probably automate this as well during building, if a repo is detected. git config --add blame.ignoreRevsFile some_ignore_file

[lynxlynxlynx]

There's a new option available that doesn't force the choice too. Could be a viable transitional thing, but also has downsides:

DerivePointerAlignment (Boolean) clang-format 3.7 If true, analyze the formatted file for the most common alignment of & and *. Pointer and reference alignment styles are going to be updated according to the preferences found in the file. PointerAlignment is then used only as fallback.

[burner1024]

For Python, once I've discovered Black, I'm using it everywhere. I might not always agree or like the style, but it's so much faster to just accept it and focus on actual development. Takes 1 week to get used to a style. Just look at this issue, 4 years and still going strong.

[lynxlynxlynx]

Yeah, because the actual work needed is not about the style and infra stuff just isn't appealing.

[burner1024]

I mean, style work isn't going anywhere. It's either automated, or done manually.