Cleanup issues of PR 8228 #8259

wwmayer · 2023-01-25T11:57:16Z

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.

donovaly · 2023-01-25T22:04:43Z

src/Gui/ExpressionCompleter.cpp

            doc = docs[idx/2];
+        }


Just for curiosity, why are curly braces necessary for single-line ifs? Up to now i did not add them because I saw no need and also because most of FC's code don't have them.

Forums discussion of clang-format: https://forum.freecadweb.org/viewtopic.php?f=10&t=72183&p=629874

The gist of it is that it's widely considered a best practice due to some very famous bugs that would have been prevented had this formatting strategy been used.

Thanks for the pointer. In this discussion I miss the info what can be harmful of omitting the braces. I don't mean when there are comments and then a single statement, but just a single statement alone, so

if (blah) do something; else if (blub) do another thing; ...

Especially when having "else if" this way improves in my opinion the readability, especially on smaller screen since it saves lines.

Besides this, I recently reverted the automatic addition of braces, not because I am opposed of requiring the braces, but it was a nightmare to work with the VC IDE with this. At first, the addition seemed convenient but on longer coding sessions VC is not able to do this right when copy/pasting code around. Often it adds a closing brace despite it is already there. This cost a lot of time since the compilation error messages don't help. So one has to carefully re-read to find additions braces. On the other hand sometimes it does not add the closing brace. Still a mystery for me. What about you, are you on VC 2022 as well? When you enabled automatic brace insertion, and code around for a while, do you encounter problems? I had a look today in forums but cannot find a way to use a clang file and only overwrite one of its setting.
I see no workaround because I cannot turn of the formatter because then the code would not be formatted at all. And having a custom clang file only for me is error-prone (different PCs, keeping it up to date.)

from what you are saying it sounds like it's messing up the file while you are coding, that does sound annoying and error prone, can't you run clang format just before committing?

Thanks for the pointer. In this discussion I miss the info what can be harmful of omitting the braces

my first guess is that later someone might forget to put braces when trying to add more lines inside the if statement, I've certainly done so myself 🤦‍♂️

In my opinion, format-as-you-type is more trouble than it is worth, in any IDE. My strategy is to literally not worry about the format at all while I am coding, and just make the code look how I like it to look. Then, before committing, I select and auto-format (or in the case of Python, use Black as a pre-commit hook).

"In this discussion I miss the info what can be harmful of omitting the braces."

The most famous case is the "Apple goto fail". There is also the python mantra "explicit is better than implicit". From a maintainability standpoint, anything that reduces the chances of misinterpreting code is a good thing.

berniev · 2023-01-26T01:28:01Z

Interesting to see the experimental clang-format spoken of like it is official. Nice!

Just a couple of cautions though:

It was previously pointed out that broadscale reformatting should be avoided for now to avoid further complicating the pending toponaming merge.

Format will eventually (I believe) be included in CI. After that local issues in the IDE become irrelevant. Do whatever you want! The point is that local issues are temporary and should not block achieving the best final outcome.

berniev · 2023-01-26T01:44:22Z

This cost a lot of time since the compilation error messages don't help

I dont use VC but CLion and VSCode both instantly alert mismatched braces when I mess up. Is VC2022 is not similar? Odd these get through to compile.

Maybe: How do I specify a clang-format file

donovaly · 2023-01-26T03:30:29Z

Interesting to see the experimental clang-format spoken of like it is official. Nice!

The point is that it is not experimental. Since VC 2019 the clang file of a project is the default formatter. You cannot disable this, except of

turning of the formatter completely -> then your code is not formatted at all. Another user had the same problems as I and turned off the formatter completely, see Fix8228 warnings #8262 (comment) and the result is that his code has no uniform formatting
using your own clang file -> I tried this as well but sine I am the stable release manager I have different PCs now (currently 4). It consumes time to maintain its own clang file and to keep it in sync on all PCs. Moreover, it would contradict the principle that everybody out using VC should have its own clang file just to be able to be productive.

berniev · 2023-01-26T03:50:07Z

At the very least you could identify your problem and maybe others have something to solve. Give some example etc. So far all we've heard is you don't like what happens.

donovaly · 2023-01-26T04:00:04Z

all we've heard is you don't like what happens.

Where did you hear this? I even approved the change to add auto brace insertion.

It took me a lot of time to realize what is going on, why I got frequent and hard to understand compilation errors. Thus I reverted the particular change to be productive again and wrote about this in the commit. I know, this was not according to our rules. But following the rules would have meant that I would not be productive for days, have to start a debate, bring examples etc. And as you see others were affected too. Thus it was quicker to "just resolve" the issue.
I was hardly criticized for my action. I can understand this. However, it is our spare time and when a simple change that only aims to add convenience for developers turn out to introduce troubles, one should quickly act. And here no code was actually changed.

berniev · 2023-01-26T04:44:59Z

It took me a lot of time to realize what is going on, why I got frequent and hard to understand compilation errors

You need to seriously revise your workflow. If you are using a moden IDE there is no way a format problem should remain undiscovered until compilation.

donovaly · 2023-01-26T05:59:16Z

You need to seriously revise your workflow. If you are using a moden IDE there is no way a format problem should remain undiscovered until compilation.

So Visual Studio 2022 is not modern? Exactly because I knew that I will get statements like this:
it is about you, you need to check...
I made the change, became productive again and others too. Yes, apparently VC 2022 has a bug, but why should all its users suffer for now?

wwmayer · 2023-01-26T13:37:35Z

Just for curiosity, why are curly braces necessary for single-line ifs?

If you have some if-else if checks on the same level and any of it requires braces then by most style guidelines it's strongly recommended to add curly braces to all of them.

Example:

if (check1)
   do_something_1;
else {
   do_something_2;
   do_something_3;
}

should become

if (check1) {
   do_something_1;
}
else {
   do_something_2;
   do_something_3;
}

If you have an if/else if/else with a comment before the actual statement then it's strongly recommended to add curly braces.

if (check1)
    // this is a comment
   do_something;

should become

if (check1) {
    // this is a comment
   do_something;
}

If you have nested if-checks then for the outer if's curly braces are strongly recommended.

if (check1)
    if (check2)
       do_something;

should become

if (check1) {
    if (check2)
        do_something;
}

or (preferred)

if (check1) {
    if (check2) {
        do_something;
    }
}

berniev's rule is even simpler because he recommends to always use curly braces. Unfortunately a lot style guidelines say something else where for single line statement the curly braces should be dropped.

donovaly · 2023-01-26T14:21:01Z

If you have some if-else if checks on the same level and any of it requires braces then by most style guidelines it's strongly recommended to add curly braces to all of them....

Many thanks. What you describe is how most of FC's code is written and also how I try to code.
Therefore I don't see why this must be changed.

The problems with changing the rules are:

When copying code around braces will be added also single-line statements like

if (bla)
    return something;

This gives you a lot of changes in PRs that have nothing to do with the actual intended code change, making it harder for the reviewers or you have to undo the automatical additions to keep the diffs small. Both ways consume spare time either of the coder or the reviewer.

on VC 2022 sometimes only the opening brace is added, but sometimes also a closing one even when there is already a closing one. This is apparently not only annoying for me but for other coders too.

All in all, the whole issue consumes a lot of our spare time, for the discussion and also to handle the code. Therefore I don't see why we need to change or style when it gives us no benefit, but additional work. We should better use our time to focus to write good code (good to understand for others, less memory consumption, fast etc.)

wwmayer · 2023-01-26T15:21:10Z

on VC 2022 sometimes only the opening brace is added, but sometimes also a closing one even when there is already a closing one. This is apparently not only annoying for me but for other coders too.

I don't use it a lot any more but sounds like the IDE is not very smart. Maybe you should open a new thread in the forum to discuss this issue.

berniev · 2023-01-26T16:01:54Z

Re single line statements, a slight clarification:

if(bla) doThis();

and

if(bla)
    doThis();

Should the first version be allowed? I would argue it should not be allowed as it hides content and makes debugging more difficult. If it is allowed then perhaps braces are superfluous.

The second case it seems has concensus for always brace.

berniev · 2023-01-26T16:03:46Z

I tried Visual Studio 2022 for Mac, but deleted it. No support for C++. I think VS priority was always C#.

chennes · 2023-01-26T16:10:12Z

Does CLion do a good job with autoformatting? I'm getting ready to migrate (I really dislike using C++ in Xcode).

berniev · 2023-01-26T16:32:24Z

CLion is, like other Jetbrains products I have used for years, very slick. Given the huge hours we spend at the screen, a hundred bucks or whatever is peanuts. I continue to be amazed how good they are. BUT CLion has some indexing/speed issues with FreeCAD - I'm talking sometimes wait 45 minutes with all cores flat out. This renders much of the refactoring assistance (one of my favourite features) unusable. I spent heaps on extra RAM - made no difference. I have several reports in. Maybe on your M1 you'll do better.
Formatting is not intrusive, works great.

WandererFan · 2023-01-26T17:43:30Z

related: #8232

berniev · 2023-01-27T01:11:57Z

@chennes clarification re CLion: The formatting is not entirely 'auto', more 'helpful'. Still hit format button from time to time.

donovaly · 2023-01-27T01:18:45Z

Should the first version be allowed?

In my opinion not. As I also argued in the forum, I don't see the need to change the formatting when > 95% of your code use already the linebreak.

The second case it seems has concensus for always brace.

Where do you see this?
Personally I am against this since it brings us no benefit. I vote to keep the status we already more ore less have and this is that: #8259 (comment)

wwmayer added 2 commits January 25, 2023 12:50

Gui: improve formatation and use of curly braces

ea526fd

Gui: fix several compiler warnings

b0a8702

github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Jan 25, 2023

wwmayer mentioned this pull request Jan 25, 2023

Expressions: path auto-completion fixes #8228

Merged

1 task

donovaly reviewed Jan 25, 2023

View reviewed changes

donovaly merged commit 0f694c3 into FreeCAD:master Jan 25, 2023

donovaly mentioned this pull request Jan 25, 2023

Fix8228 warnings #8262

Merged

1 task

wwmayer deleted the cleanup_PR branch January 26, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup issues of PR 8228 #8259

Cleanup issues of PR 8228 #8259

wwmayer commented Jan 25, 2023

donovaly Jan 25, 2023

chennes Jan 25, 2023

donovaly Jan 25, 2023

adrianinsaval Jan 26, 2023

adrianinsaval Jan 26, 2023

chennes Jan 26, 2023

WandererFan Jan 27, 2023

berniev commented Jan 26, 2023

berniev commented Jan 26, 2023

donovaly commented Jan 26, 2023

berniev commented Jan 26, 2023

donovaly commented Jan 26, 2023

berniev commented Jan 26, 2023

donovaly commented Jan 26, 2023

wwmayer commented Jan 26, 2023

donovaly commented Jan 26, 2023

wwmayer commented Jan 26, 2023

berniev commented Jan 26, 2023

berniev commented Jan 26, 2023

chennes commented Jan 26, 2023

berniev commented Jan 26, 2023

WandererFan commented Jan 26, 2023

berniev commented Jan 27, 2023

donovaly commented Jan 27, 2023

Cleanup issues of PR 8228 #8259

Cleanup issues of PR 8228 #8259

Conversation

wwmayer commented Jan 25, 2023

donovaly Jan 25, 2023

Choose a reason for hiding this comment

chennes Jan 25, 2023

Choose a reason for hiding this comment

donovaly Jan 25, 2023

Choose a reason for hiding this comment

adrianinsaval Jan 26, 2023

Choose a reason for hiding this comment

adrianinsaval Jan 26, 2023

Choose a reason for hiding this comment

chennes Jan 26, 2023

Choose a reason for hiding this comment

WandererFan Jan 27, 2023

Choose a reason for hiding this comment

berniev commented Jan 26, 2023

berniev commented Jan 26, 2023

donovaly commented Jan 26, 2023

berniev commented Jan 26, 2023

donovaly commented Jan 26, 2023

berniev commented Jan 26, 2023

donovaly commented Jan 26, 2023

wwmayer commented Jan 26, 2023

donovaly commented Jan 26, 2023

wwmayer commented Jan 26, 2023

berniev commented Jan 26, 2023

berniev commented Jan 26, 2023

chennes commented Jan 26, 2023

berniev commented Jan 26, 2023

WandererFan commented Jan 26, 2023

berniev commented Jan 27, 2023

donovaly commented Jan 27, 2023