Code cleanup/style

[Baklap4]

Do you want to request a feature or report a bug?

Question/feature

What did you expect to see?

I expected some clean code all written in the same style.

This issue is open to discussion. Let's try to get to a point where we can say: let's do performous this way.

Suggestions

1. Make/use/find code conventions for latest c++
2. Use a tool to force these conventions on us, popular tools are; editorconfig, cpplint
3. Use these tools within the CI
4. Treat warning as errors, forced upon us by cmakelist
5. Tabs vs spaces, spaces vs tabs
6. .. some other stuff?

LEt's see what everyone wants to bring in :)

Again this is open to discussion

@performous/core-owners @performous/contributors

[OznOg]

yes unifying would be a good idea; as tools there is also uncrustify, cang-format (indent). I'm not a great fan of tabs, but seems to be quite "standard" in the code, so let's not mess everything up just for spacings. -Wall -Wextra -Wall -Werror should already be fine to have. The code may also be passed to coverity, this is available online for opensource projects, the tool may find some relevant problems. One really nice to have would be unit tests and/or validation tests, so that we are more confident with the code being pushed.

[Tronic]

Performous coding style: https://github.com/performous/performous/wiki/Coding-style

I would avoid all tools that try to make lines of particular length because they lose all the thought that has been put into manual formatting. In particular, I dislike splitting if and what follows it like return something on separate lines, especially when many similar ifs follow each other because the interleaved conditions and responses are far harder to read than almost identical lines following each other, and if there are no curly braces (which then add more noise) it is also quite easy to mess up.

Also worth a notice is that all auto-indent tools have been more or less broken by design. Most of them think that a tab is equivalent to some integer number of spaces, which is just wrong (my editor is set to use tab width of π columns to avoid such misconception). Also, none of them know of the small indent (two spaces) convention used for splitting long lines (previously also case, public, private but because of tool issues we no longer indent them).

Overall, I find Performous code style quite consistent, and thus see only marginal use for extra linting, especially since compilers are quite good at finding issues nowadays. Enforcing warning-free compiles, consistent indentation, removal of end-of-line whitespace and other such things in CI couldn't hurt, as long as the tools don't break existing code and practices.

[twollgam]

I would like to change the coding style from tabs to 4 spaces and change the small indent to normal indent. I am not the only one who has problems with tabs.

If we decide for changing this I would offer:

• Change the coding style in the wiki.
• Add a git hook that enforces using of 4 spaces and deny tabs (if possible).
• Commit all needed conversions.

[Tronic]

If going for spaces, I'd suggest 2 spaces everywhere. It simplifies the "small indent" that some tools cannot otherwise support, as then everything is 2 spaces. Also this narrower width is very popular and almost ubiquitous with many languages. C and C++ of course come in all different styles from tabs to 2, 4, 5 or 8 spaces, and historically tabs have been a good solution in that everyone can choose how wide they want to view the indents because there never was agreement on how many spaces should be used. A notable case for four spaces is Python PEP8, which means that most Python code uses that width, although recently I've seen projects move to two spaces with Python too, probably because of Javascript and other webdev that did so in recent years.

Converting the indents destroys git blame for the whole project. Dunno if there is any reasonable way to rewrite git history such that the reindentation does not show up in blame.

Care must be taken to still disallow alignment with open braces and other such stupidities still widely used in C++ tooling. Also I would do the fixing by search & replace and avoid running any autoformat tools as the code is carefully formatted and those tools would destroy any careful consideration done by developers on where to put newlines.

For better compatibility with tooling like smart indent in editors, one might consider breaking anything within braces in this style

void f(
  int a,
  int b
);

Or oneliner

void f(int a, int b);

But NOT anything like

void f(int a,
  int b);

As an alternative solution, implement a git hook that enforces tabs only at beginning of lines (not mixing with spaces), no more than two consecutive spaces allowed, no whitespace at end of line, file must end with a single newline, so that people actually configure their editors correctly. All editors can support tabs and whitespace normalization just fine but developers often won't bother to setup their editors correctly. Making it impossible to commit broken whitespace is indeed a good idea.

[Lord-Kamina]

Personally, I've always been a fan of tabs because I think they're faster and neater.

Now, even if nobody else agrees with me. Is it really worth losing git blame as @Tronic says?

[Tronic]

@Lord-Kamina I prefer tabs in C++ too, but have no strong preference either way.

[twollgam]

Hmm, does the -w option for git blame help?

[Tronic]

@twollgam Could you check how that works with Github diffs, VS Code Git Lens and other such tooling where you cannot directly add the flag?

[twollgam]

For github I found this: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

[twollgam]

But it looks like there is no decision for a change. How ever a server side git hook that rejects wrong indent would help. (I have no experience with server side hooks and do not know what there is possible). For client side I would use a hook that converts the spaces into tabs.

[Tronic]

Could also implement indent check in Github CI, such that PRs cannot be merged with malformed whitespace. Client-side whitespace cleanup might be good enough too, although automatic conversion of indents is known to be difficult (in general, you cannot safely convert any consecutive N spaces into a tab character, nor vice versa, especially if the developer is not following the style guide).

[Lord-Kamina]

But it looks like there is no decision for a change. How ever a server side git hook that rejects wrong indent would help. (I have no experience with server side hooks and do not know what there is possible). For client side I would use a hook that converts the spaces into tabs.

I personally use an editor that lets me specify tab width and convert back and forth between spaces and tabs.

[OznOg]

I personally use an editor that lets me specify tab width and convert back and forth between spaces and tabs.

I also do, but problems come when I move from a project to another which obviously do not have same rules. An automatic formater would suit me, even if I agree that killing the git blame is a real concern. If it was possible to have an automatic formated on the edited code that would suggest indentation and let you rework it (but work for 90% of the commits), it would be rather helpful, but probably hard to write...

[Baklap4]

Within most of my projects i use editorconfig combined with treat warnings as errors and listing all style related stuff as a warning. This way it can be detected in an automated build within the CI and it also sets up the editor of your choice if you have then extension or plugin installed.

A good addition is the git commit hooks to prevent malformed styling issues. This can be done with a simple regex to detect tabs vs spaces

in terms of tabs vs spaces i'm content with both, but prefer tabs

[twollgam]

I opened the PR #717 that contains git hooks. They check if spaces are used instead tabs and if there is whitespace at end of a line.

Server-side hooks can't be used on github IIRC so we need to do checks in ci.