Coding. style

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Coding. style

Post by FreddyFreddy »

jnxd wrote: Wed Apr 20, 2022 5:31 am I recall I was asked to revert he auto in at least one PR.
I can't understand why. Does it make sense to you? The type has already been declared, so re-typing the type is a waste of space and inviting typos etc. Auto makes it easier to comprehend. You don't have to go through all the mental hoops all over again.
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Coding. style

Post by FreddyFreddy »

jnxd wrote: Wed Apr 20, 2022 5:31 am Definitely high priority. The former at best makes the code less readable and at worst ignores whatever value the local variable had, causing all kinds of bugs.
Easy. Just change the name of the duplicate variable to some arbitary name.

The latter (leading _) I understand is all about not polluting the global space (plenty of that here). Not hard to fix, but might need some concensus on naming convention. Any inputs?
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Coding. style

Post by FreddyFreddy »

jnxd wrote: Wed Apr 20, 2022 5:31 am it's already quite a daunting task that has the potential to break things in unexpected ways for months on end if done incorrectly.
I somewhat disagree. Many are just simple replace. In an IDE trivial. (With enormous respect) surely way below the pay grade of a founder.
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Coding. style

Post by FreddyFreddy »

jnxd wrote: Wed Apr 20, 2022 5:31 am so your PR's would be more than welcome.
Thanks for the support! Will get there soon enough.
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Coding. style

Post by FreddyFreddy »

jnxd wrote: Wed Apr 20, 2022 5:31 am Uwe and Werner (among others) have already made a whole lot of changes by removing unnecessary inputs and modernizing the code.
Yes, looking at the code there have been some updates, but (and I'm almost loath to say this for fear of being flamed) you only have to look at the code to know much of it is legacy style. As Werner said, much of it was written in C++98 and is up to 20 years old. 5000 line files? Tangled responsibilites? Polluted global space. These things were the norm when written, but the world has moved on a bit ... (please don't flame).
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Coding. style

Post by wmayer »

jnxd wrote: Wed Apr 20, 2022 5:31 am Quite a few of these can be trivial, but keep in mind that an auto keyword makes the code a little bit unreadable since the type is not explicitly visible to the developer. I recall I was asked to revert he auto in at least one PR.
Really? IMO, using auto for iterators is one of the best possibilities to use it because you don't have to repeat all the template parameters of the container.
FreddyFreddy wrote: Wed Apr 20, 2022 7:23 am I can't understand why. Does it make sense to you? The type has already been declared, so re-typing the type is a waste of space and inviting typos etc. Auto makes it easier to comprehend. You don't have to go through all the mental hoops all over again.
+1
FreddyFreddy wrote: Wed Apr 20, 2022 7:29 am The latter (leading _) I understand is all about not polluting the global space (plenty of that here). Not hard to fix, but might need some concensus on naming convention. Any inputs?
Concrete example?
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: Coding. style

Post by jnxd »

wmayer wrote: Wed Apr 20, 2022 7:56 am
jnxd wrote: Wed Apr 20, 2022 5:31 am Quite a few of these can be trivial, but keep in mind that an auto keyword makes the code a little bit unreadable since the type is not explicitly visible to the developer. I recall I was asked to revert he auto in at least one PR.
Really? IMO, using auto for iterators is one of the best possibilities to use it because you don't have to repeat all the template parameters of the container.
I do recall I was asked to revert. In that case though there weren't too many template parameters to be provided, and I was replacing the explicitly stated type with auto. I believe the precedent set was "you can use auto in new loops, but if a type is already provided, no need to change it". It might also not have been for an iterator, in which case the argument weakens a little bit.
uwestoehr wrote: Hi
Hi Uwe. Mind hopping in with your inputs? Do you recall what the PR was where we talked about it?
My latest (or last) project: B-spline Construction Project.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: Coding. style

Post by jnxd »

FreddyFreddy wrote: Wed Apr 20, 2022 7:35 am Many are just simple replace. In an IDE trivial. (With enormous respect) surely way below the pay grade of a founder.
Oh I'm far from a founder :D (not even getting into the paygrade part :lol:).

As for the ease of replacing, that may be the case but I'd still be cautious in my steps. Regardless, I see you're set on your plan so not gonna stop you.
My latest (or last) project: B-spline Construction Project.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: Coding. style

Post by jnxd »

FreddyFreddy wrote: Wed Apr 20, 2022 7:29 am The latter (leading _) I understand is all about not polluting the global space (plenty of that here). Not hard to fix, but might need some concensus on naming convention. Any inputs?
No inputs as such. The convention changes module to module, and only some modules have a set guideline (which is not even strongly imposed).
My latest (or last) project: B-spline Construction Project.
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Coding. style

Post by wmayer »

jnxd wrote: Wed Apr 20, 2022 9:11 am Mind hopping in with your inputs? Do you recall what the PR was where we talked about it?
https://github.com/FreeCAD/FreeCAD/pull ... r757918354

This is the code block:

Code: Select all

            // shells are already closed - add them directly
            for (TopoDS_Shape& s : shells) {
                mkSolid.Add(TopoDS::Shell(s));
            }
I don't see a problem with readability if auto instead of TopoDS_Shape was used because the call of TopoDS::Shell makes it obvious that s must be a TopoDS_Shape
Post Reply