about today's huge auto change

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!
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

about today's huge auto change

Post by uwestoehr »

Looking at the huge PR that was just merged:
https://github.com/FreeCAD/FreeCAD/pull/7481

Was there a discussion? Despite I personally enjoy to use auto, I have @wmayer's words in mind that code should also be readable well without having an IDE.
In the PR, there are several cases where with the auto type setting it is not clear what type the variable has just by looking at the code.

Here an example:

Code: Select all

 -  std::map<std::string, std::string>::const_iterator ht = config.find("HiddenDockWindow");
 +  auto ht = config.find("HiddenDockWindow");
("ht" is already not a meaningfule variable name and the info that it is a std::map was useful in my opinion.)

I am not opposed, but want to have a discussion if we want to go this way.

@chennes , @berniev
User avatar
chennes
Veteran
Posts: 3879
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: about today's huge auto change

Post by chennes »

I reviewed this PR and in every case decided that the type was easily determined either directly from the line, or from one or two lines above. I believe that the case you cite is one where you need one more line of context to determine the variable's type.

ETA: with both lines, I believe the type is abundantly clear:

Code: Select all

    const std::map<std::string,std::string>& config = App::Application::Config();
    auto ht = config.find("HiddenDockWindow");
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: about today's huge auto change

Post by wmayer »

To be honest I am not a friend to use auto everywhere. Where I find it really useful is a replacement for complex iterator types of containers like in the example you posted:

Code: Select all

std::map<std::string, std::string>::const_iterator ht = config.find("HiddenDockWindow");
auto ht = config.find("HiddenDockWindow");
User avatar
chennes
Veteran
Posts: 3879
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: about today's huge auto change

Post by chennes »

I agree with you, and I worked with @berniev to make sure this PR focused on cases where the type was literally repeated on the same line, e.g.

Code: Select all

QPushButton* button = new QPushButton()
becomes

Code: Select all

auto button = new QPushButton()
Even that isn't a hill I will die on, though I do think it's a useful (if somewhat marginal) improvement. The thing in this PR that I was most interested in was the ranged for loop replacement, which I believe is a clear improvement in basically all cases.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Re: about today's huge auto change

Post by berniev »

I had just completed a curteous detailed response and this f'ing stoopid pos bbs demanded re-login before submitting and the reply was discarded.
I note this does not appear to be a public discussion.
several cases where with the auto type setting it is not clear what type the variable has just by looking at the code.

Here an example:
Prior to my and wmayer commits was:

Code: Select all

332: const std::map<std::string,std::string>& config = App::Application::Config();
333: std::map<std::string, std::string>::const_iterator ht = config.find("HiddenDockWindow");
The definition is in the immediately prior line. It can't get any closer than that.
I worked with @berniev to make sure this PR focused on cases where the type was literally repeated on the same line
That's not entirely correct. We certainly agreed on only auto. I had previously added drive-by fixes. It was only later you asked auto loops and single line auto be kept separate -- due to time taken to review prev PR, which is fine.

In response to this I sought clarification on a number of things you had been requiring. The response was unhelpful, and unfortunate.

I looked at the code in actual use in FreeCAD, particularly re @wmayer, @realthunder and @chennes. I found all use auto liberally and in a much more relaxed manner than being suggested here. They all regularly auto on a var which is a class var. They all auto loop on a method return value, even sometimes where the method is outside the current class.

So the rhetoric and reality are different.


Anyhow, much to do. I propose:

- Permit auto on class var (from within the class) [existing practise]
- Permit auto loop on method return value (from within the same class) [existing practise]
- Require * in auto on pointer [clang-tidy default]
- Do not require const on auto when making iterator on var. [Compiler will choose const-iterator or iterator]
- Do not require const on auto where referenced var is already const. [Compiler will make it const]
- Do not require const auto & var everywhere possible. [Sometimes with simple types auto var makes for less copying. IDE with clang-tidy helps]

- In a large PR review do not quibble about very minor formatting issues. [WOFTAM There's already zillions of them in the codebase and all will one day be wiped out by a global reformat (I hope!)]
- In a PR review don't request incidental fixes [Conflicts demand on coder to stay on-topic]

- Have discussion, but don't sweat every minor detail. Let's have proper big picture discussion. Currently there is no plan, no leadership, for the most of us to follow. IMHO there is a huge amount to do. This is the lowest hanging fruit, the simplest part of modernisation.
heda
Veteran
Posts: 1348
Joined: Sat Dec 12, 2015 5:49 pm

Re: about today's huge auto change

Post by heda »

"bbs demanded re-login before submitting and the reply was discarded."

yeah, it is a pita when that happens - there is some light in the tunnel though :-)

I have found that in most cases one can just use the back-button on the browser and one gets the written text back, just copy it and make the login and then paste and all is good.

in any case, i now have the habit of pressing preview directly (as in before starting to type) on any post made, it is an quick and easy way to see if you need to login again...
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Re: about today's huge auto change

Post by berniev »

heda wrote: Sun Sep 18, 2022 6:36 am "bbs demanded re-login before submitting and the reply was discarded."
I have found that in most cases one can just use the back-button on the browser and one gets the written text back, just copy it and make the login and then paste and all is good.
Thanks for the thought.
That's been suggested before and in my case doesn't work. I suspect being on a Mac with privacy and security tied down tight there's a cookie getting wiped, or suchlike. It only occurs after some time, maybe an hour, or less. PITA.
chrisb
Veteran
Posts: 53929
Joined: Tue Mar 17, 2015 9:14 am

Re: about today's huge auto change

Post by chrisb »

berniev wrote: Sun Sep 18, 2022 6:43 am I suspect being on a Mac with privacy and security tied down tight there's a cookie getting wiped, or suchlike. It only occurs after some time, maybe an hour, or less. PITA.
I check on login the "Remember me" box and that helps a lot. However, it is IP based, and if your ISP gives you a new IP it may not work.
The builtin browser (Safari) discards indeed all inputs made if I go back. Firefox is in this respect much better; also with inadvertently closed tabs.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Re: about today's huge auto change

Post by berniev »

chrisb wrote: Sun Sep 18, 2022 9:17 am However, it is IP based, and if your ISP gives you a new IP it may not work.
The builtin browser (Safari) discards indeed all inputs made if I go back. Firefox is in this respect much better; also with inadvertently closed tabs.
We spoke about this ages (Mar/Apr?) ago. The future is more privacy. How do we fix this? Kill/extend the timeout? Give me the keys?
chrisb
Veteran
Posts: 53929
Joined: Tue Mar 17, 2015 9:14 am

Re: about today's huge auto change

Post by chrisb »

berniev wrote: Sun Sep 18, 2022 9:51 am The future is more privacy.
I don't know the exact details how "Remember me" works. But as HTML connections are stateless, you somehow have to store somewhere information about the connection. This is independent from the browser used.

Concerning storing the edited text, so that you can get back to it: that's only local on yur computer. If you don't want anybody else to see your browser history, you must either lock your computer or use a private tab for the FreeCAD forum, which you close before leaving the computer.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
Post Reply