Coding style for C++

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
User avatar
vanuan
Posts: 153
Joined: Wed Oct 24, 2018 9:49 pm

Coding style for C++

Postby vanuan » Sun Aug 02, 2020 5:56 pm

The distinctive feature of any mature project is its established coding style and automatic verification by CI. I see that FreeCAD still lacks any linter of coding style.

One such a linter is clang-format. Its available in all major platforms - VS C++ on Windows, XCode on OS X and as a standalone CLI tool on Linux.

There are 2 points to discuss though - which style is being used and how to integrate it into CI/CMake/IDE.

For the style, it seems to be LLVM with modifications. As for the CI integration, I can hack something out.

The most difficult part, of course, it getting everyone onboard with this.

So, who's against, who's onboard?
User avatar
vanuan
Posts: 153
Joined: Wed Oct 24, 2018 9:49 pm

Re: Coding style for C++

Postby vanuan » Sun Aug 02, 2020 5:59 pm

User avatar
uwestoehr
Posts: 1634
Joined: Sun Jan 27, 2019 3:21 am

Re: Coding style for C++

Postby uwestoehr » Sun Aug 02, 2020 6:35 pm

vanuan wrote:
Sun Aug 02, 2020 5:56 pm
So, who's against, who's onboard?
me :+1:
User avatar
DeepSOIC
Posts: 7658
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Coding style for C++

Postby DeepSOIC » Sun Aug 02, 2020 9:46 pm

Harmonizing the coding style will cause a lot of merge conflicts for big-effort branches like realthunder's. My ConstraintSolver will be affected noticeably, but not critically, as most of the code is in a new module. So i'm a bit against.

If it has to be done, I think sooner is better than later, the situation is likely to get worse over time, not better.


vanuan wrote:
Sun Aug 02, 2020 5:56 pm
and automatic verification by CI.
I am somewhat strongly against. To every rule, there will always be exceptions. From code generated by generators, to special situations where rules should be violated for good. Please don't make it an unbreakable law. It should help, not harm.
User avatar
vanuan
Posts: 153
Joined: Wed Oct 24, 2018 9:49 pm

Re: Coding style for C++

Postby vanuan » Sun Aug 02, 2020 10:28 pm

DeepSOIC wrote:
Sun Aug 02, 2020 9:46 pm
Harmonizing the coding style will cause a lot of merge conflicts for big-effort branches like realthunder's. My ConstraintSolver will be affected noticeably, but not critically, as most of the code is in a new module. So i'm a bit against.

If it has to be done, I think sooner is better than later, the situation is likely to get worse over time, not better.
Another reason to merge PR sooner rather than later. If something takes a big effort, you can always introduce a runtime feature flag and merge incrementally.

DeepSOIC wrote:
Sun Aug 02, 2020 9:46 pm
vanuan wrote:
Sun Aug 02, 2020 5:56 pm
and automatic verification by CI.
I am somewhat strongly against. To every rule, there will always be exceptions. From code generated by generators, to special situations where rules should be violated for good. Please don't make it an unbreakable law. It should help, not harm.
Do you prefer to argue in PR comments what deserves to be a special case? Or would you rather make the tooling take care of it? I doubt you ever make a comment about the generated code. Why even check-in the generated code? Every tool has plenty of options to ignore some rules in some files.
User avatar
DeepSOIC
Posts: 7658
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Coding style for C++

Postby DeepSOIC » Sun Aug 02, 2020 11:09 pm

vanuan wrote:
Sun Aug 02, 2020 10:28 pm
Do you prefer to argue in PR comments what deserves to be a special case? Or would you rather make the tooling take care of it?
I don't understand you. You seem to imply that having no tool will cause heated discussions on what should be an exception or not... while without the tool, there are no discussions, because the violations are not exposed...
The tooling will not by itself decide ("take care") of what should or what should not be an exception for us - it's our very manual job of setting up the tooling... Or am i missing something?

donovaly commented 5 days ago

Oh no, I opened Pandora's box ;-).
Indeed. All I see is unnecessary complication. All was easy before. Now, suddenly, if i want to add an exception, I have to fight yet another complicated system. One invented just for making code look good, that happens to prevent my code from looking good.

Though, it's hard to estimate the situation until the system is put into action, and I bump into a worthy exclusion.
User avatar
vanuan
Posts: 153
Joined: Wed Oct 24, 2018 9:49 pm

Re: Coding style for C++

Postby vanuan » Sun Aug 02, 2020 11:18 pm

DeepSOIC wrote:
Sun Aug 02, 2020 11:09 pm
I don't understand you. You seem to imply that having no tool will cause heated discussions on what should be an exception or not... while without the tool, there are no discussions, because the violations are not exposed...
I imply that without the tooling it's getting annoying. One thing when the tool says you did a mistake, another thing when the person asks you to fix something trivial which was not worth waiting for another round of review. Also, in the first case, you fight with a tool, in the second case you fight with a person. I see it as a sign of immature project.
mikeprice99
Posts: 77
Joined: Sat Jun 27, 2020 9:06 am
Location: Dorset, England

Re: Coding style for C++

Postby mikeprice99 » Mon Aug 03, 2020 10:05 am

vanuan wrote:
Sun Aug 02, 2020 5:56 pm
... and how to integrate it into CI
Maybe I don't understand how the CI works in relation to PRs, but won't CI compilations fail because of all existing coding style failures?
User avatar
bernd
Posts: 10470
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland

Re: Coding style for C++

Postby bernd » Mon Aug 03, 2020 11:00 am

DeepSOIC wrote:
Sun Aug 02, 2020 9:46 pm
Harmonizing the coding style will cause a lot of merge conflicts for big-effort branches like realthunder's. My ConstraintSolver will be affected noticeably, but not critically, as most of the code is in a new module. So i'm a bit against.

If it has to be done, I think sooner is better than later, the situation is likely to get worse over time, not better.
This is exactly the problem on code formating. In FEM if I have done heavy code formating I was the one who has helped a lot in rebasing the branches which where not merged. But once done IMHO the code looks much better and most important for me it is much simpler to read and understand. Most big PR do relate to realthunder. Best is to convince him to use the codeformating too.

DeepSOIC wrote:
Sun Aug 02, 2020 9:46 pm
vanuan wrote:
Sun Aug 02, 2020 5:56 pm
and automatic verification by CI.
I am somewhat strongly against. To every rule, there will always be exceptions. From code generated by generators, to special situations where rules should be violated for good. Please don't make it an unbreakable law. It should help, not harm.
+1 to DeepSOIC. For python and FEM I have been used code formating checking for years. No we should not do automatic verification
wmayer
Site Admin
Posts: 16067
Joined: Thu Feb 19, 2009 10:32 am

Re: Coding style for C++

Postby wmayer » Mon Aug 03, 2020 4:14 pm

About the code formatting I wonder whether it's possible to leave some flexibility and not to have define every little detail. If you look at https://clang.llvm.org/docs/ClangFormat ... tions.html you will see how many things can be handled differently and for many of them I personally don't care what's used (e.g. all the Align* options).

For me the most important point is readability. Nothing is more tedious and tiring to review unreadable code because of poor formatting. This does not only considerably reduce maintainability but also increases the likelihood to overlook serious bugs or later introduce them unintentionally.

At the moment a lot of FreeCAD code looks very squashed where adding some newlines would already help a lot to more easily understand what it is supposed to do. Therefore I suggest e.g. for lengthy (and nested) if-else cascades to put the else keyword to the next line after a brace.

Or often there are if-else constructs where in one else-branch curly braces are used because the body consists of several lines but in the next branch they are omitted because it consists of a single line. In this case most coding styles recommend to use curly braces for all bodies if at least one body needs it.

Or if a branch of an if-else block has only one statement but an additional comment curly braces should be used, too.
Indeed. All I see is unnecessary complication. All was easy before. Now, suddenly, if i want to add an exception, I have to fight yet another complicated system. One invented just for making code look good, that happens to prevent my code from looking good.
We need at least some coding styles where the readability is harmed at the moment.