fixClazy warnings #71
Conversation
|
Well that makes me a little sad. There was a time when Qt said to prefer their containers and for loops to those of the standard libraries. Times change. However if you try to keep up I fear you will just spend a lot of time without any improvement as far as the user is concerned. Those are changes just for changes sake. Go ahead we have a different maintenance philosophy however. |
I will mitigate this just a little bit. I do not plan to change every foreach and every container to std:: |
|
@githubdoe I'm working on some other simple cleanups that I will share once this is merged and I have 2 questions:
|
|
They must be kept. That is the original source that Mike Peck provided to do PSI analysis. They have advanced routines in them that I never implemented but planned to if it becomes important. |
|
@githubdoe anything to add about |
|
I don't find any zernEnableUpdateTime in the code. Where do you see it? |
|
Ok I found it. Yes it can be removed. Probably was meant to help set a timer that would trigger to update a display but either never got implemented or was no longer needed. |
I agree it's not directly usefull to change code that worked but still, I spotted some dead code, file compiling while unused and minor (non-)issues. The tool even spotted some |
| QMap<double,int > histo(const QList<double> data, int bins){ | ||
| auto minma = std::minmax_element( data.begin(), data.end()); | ||
| double range = *minma.second - *minma.first; | ||
| double histInterval = range/bins; | ||
| QMap<double,int> intervals; | ||
| for (int i = 0; i < bins; ++i) | ||
| for (int i = 1; i <= bins; ++i) |
There was a problem hiding this comment.
I think the old code with i=0 was fine. The new code is also fine. I have no preference so in general we shouldn't change this. But it's fine.
There was a problem hiding this comment.
Yes I agree with you and try to restrain myself for every little changes I see. However here there was a i+1 in each loop.
As we are used to =0;<n and to =1;<=n I preferred the latest in this case. I know it's only style and I should just have let it alone...
|
About the
|
|
I agree with George. I think it is very risky to modify all of that code for no good reason. I'm especially concerned when I just now discover from the conversations here that some reasons for change was efficiency reasons as indicated by a warning. Stop doing any of those please. |
|
I saw the adding "const" in various places and don't understand the need. Please explain. |
|
IIRC The astig plots computation methods change and depends on the number of rotation angles used. That is because it wants to draw a circle through the averages. However if only 2 rotations were used the method of finding a circle is different than if there are more rotations. So you need different number of rotations to full test it. |
|
The keys thing may be inefficient but it does not matter. It is only used a few times for a few data points and is not part of a huge loop. Don't worry about those. |
|
Never worry about efficiencies. You likely will not make the right choices until you run a performance analyzer to discover where they matter. However I was never able to get the performance analyzer to work with the Qt IDE. I don't remember why. |
|
Now I remember . Performance can only be run on a Linux kernel. |
Every variable that is created and only accessed for read but never write again should be declared
Thanks for feedback. So I did some more tests with rotation and still did see no difference. I will push investigation further because I want to understand.
I agree it's not a huge impact for those. However the warning was accurate and I had choice between ignore/mute warning or change the code. The modifications being cheap surgical changes reviewed and tested I choose this path. If you prefer me to mute those I can let the code as it is. Note I will do a detailed explantation for each change in the other PR (#72). Kindly let me know there which changes you would like to keep and which you would like me to drop. It's here
I agree and this was the plan. I was not going to do anything blind without analysis. Maybe I won't be able to get the analyser working, maybe I will find your code has no major performance problem and maybe I will find something. In all case you will get last word for merge and I will value the learning as I never done that in this context before. I'm more used to chase nano-amps of power consumption in MCUs. |
fix 6 occurences of
[-Wclazy-container-anti-pattern]I removed foreach loop each time because it's not recommended anyway anymore.
I tested the impact on the different windows except "artificial null error margin" (nullvariationdlg) as I did not understand how to use it. I have seen no obvious effect.