Agree upon and apply stylistic guidelines for C++ code

[rdmark]

This ticket is for settling on shared stylistic guidelines for C++ code in this project, and apply them consistently across the codebase.

There is a clang format file in the repo already:
https://github.com/PiSCSI/piscsi/blob/main/cpp/.clang-format

Action items:

• Decide if .clang-format is the best way to capture format guidelines
• Evaluate and adjust the guidelines accordingly
• Decide on the mode of enforcement (linter as part of the CI pipeline?)

[uweseimet]

I will collect some of my thoughts in this comment (and potentially update this comment).

• I recommended against changing code in the CI pipeline. The code should be formatted in the IDE only. Otherwise things become confusing, because what you see in your IDE does not reflect the final result anymore. And somehow the changes applied in the pipeline would have to be propagated back into your local sources. Does not sound appealing to me.
• Any style we agree on has to be supported by the IDEs used in this project. In my case it is Eclipse CDT.
• There should also be guidelines for Python and HTML etc., not just for C++.
• It would be helpful to see any style candidate applied in a temporary branch, in order to see how it really looks like.
• IMO the formatting of most of the non-legacy code (i.e. most of what is not in hal/ or related to the X68000) is already consistent. Any style we apply should take this into account by being close to the formatting already in use.

[rdmark]

There should also be guidelines for Python and HTML etc., not just for C++.

We do this for all Python and CSS code already, and it's part of the CI pipeline for every PR. For Python we use black for formatting, and flake8 for linting. For CSS, we use prettier for formatting, and stylelint for linting. If new code does not conform with either, we fail the pipeline.

I'm not aware of a stable linter for HTML that is jinja2 aware at this point.

This is the CI workflow, if you're interested:
https://github.com/PiSCSI/piscsi/blob/main/.github/workflows/web.yml

[rdmark]

I recommended against changing code in the CI pipeline. The code should be formatted in the IDE only.

I agree. In my opinion, the pipeline should report on non-compliant code, but not automatically modify code.

The linter/styler should be integrated in the IDE, or a CLI tool, or both.

[uweseimet]

When looking at the Eclipse settings, there are for built-in profiles: K&R, BSD/Allman, GNU, Whitesmiths. I don't know what exactly the differences are, because as usual in an IDE you can extensively customize everything. There are hundreds of individual formatting settings for that.

[dialtr]

I am big fan of clang-format and the somwhat related cpplint program. I use CLion or vim. CLion has good plugins for clang-format and cpplint. I found this plugin for Eclipse using a cursory Google search:

https://github.com/wangzw/CppStyle

But as I am not an Eclipse user, I haven't checked into it. It might be worth evaluating.

I concur with all your sentiments above. The pipeline should not reformat, but should (eventually) report on noncompliance. I say "eventually" because it might be disruptive to turn it right away unless the plan is to perform a blanket reformatting of the entire codebase (which would also seem disruptive.)

I wonder how close most of the code is to the existing clang-format?

[uweseimet]

I would not like to use an IDE plugin. Anything that is not built-in can be discontinued any time. We cannot expect that there are predefined styles in each IDE that result in the same formatting. My experience from other languages is that you have to customize the settings of each IDE in order to get something that actually is the same everywhere. This is why IMHO getting this ticket resolved is not that straightforward.
As already mentioned, reformatting the whole codebase should not be required, because most of it is already quite consistent as far as I can tell. At least I tried to be consistent with everything added in the last 2 or three years, which is probably about 70-80% of the existing code.

[dialtr]

At work (or on projects) I generally always use the command-line tool.

I just created a throwaway branch and reformatted the tree using the existing clang-format file. It was very disruptive, touched 165 files, and broke the build (probably because of reformatting macros, but not sure.)

I thus completely agree that this is not straightforward at all. Perhaps a policy could be put in place for future development and then older files updated when there is some refactoring.

As much as I like consistency, this codebase is already pretty consistent and I think the costs (making diffs harder to do over time) may outweigh the benefits.

[dialtr]

Also, even though the IDE plugins are usually wrappers around the more stable command line tools. I concur that third-party plugins are usually not well-supported. A good reason not to use them.

[uweseimet]

By the way, the formatting I have been using for this project is a style that I tried to deduce from what what appeared to be the least outdated code in the project. I did not have any particular preference, and just tried to keep the formatting as it was.
Where is the AI that looks at the existing code and configures our environments to reflect this style? ;-)

[uweseimet]

What we should (also) do, as part of a seperate ticket: Make the naming more consistent. Classes are usually camel case, except for SCSIHD, SCSIHD_NEC, SCSICD. These should be changed to ScsiHd, ScsiHdNec, ScsiCd.

[uweseimet]

Regarding formatting I have created an Eclipse formatter that almost exactly fits the existing style. I will apply it to new files first, everything else might slightly change gradually. The most notable change is probably to consistently have blanks instead of tabs for indentation.
We should consider checking in the formatting configuration file. As far as I know Eclipse formatting rules can be imported into Intellij IDEA.