added circular grid to ronchi and foucault images #335
Conversation
|
@githubdoe your blink function has been merged into master. Kindly have a look at the build warning in "files changed" tab once all automated builds are finished. They should all be fixable and fixed (except the for loop with floats which can be discussed). |
Cpp-Linter Report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (stepSizeMM <= 0) return; | ||
|
|
||
| // 4. Draw Rings and Labels | ||
| for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) { |
There was a problem hiding this comment.
clang-tidy diagnostic
foucaultview.cpp:217:5: warning: [clang-analyzer-security.FloatLoopCounter]
Variable 'currentMM' with floating point type 'double' should not be used as a loop counter
217 | for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) {
| ^ ~~~~~~~~~ ~~~~~~~~~
foucaultview.cpp:217:5: note: Variable 'currentMM' with floating point type 'double' should not be used as a loop counter
217 | for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) {
| ^ ~~~~~~~~~ ~~~~~~~~~
There was a problem hiding this comment.
I think we should leave this code as is. I thought I'd rewrite it as an integer loop like this:
for (int rPx = stepSizeMM / mirrorRadiusMM * maxPixelRadius; rPx <= maxPixelRadius; rPx += stepSizeMM / mirrorRadiusMM * maxPixelRadius) {
double currentMM = rPx * mirrorRadiusMM / maxPixelRadius;
And we could do the above replacement and it would work fine. I just swapped the 2 variables that are looping through (rPx and mirrorRadiusMM) and fixed the scaling for it to work the other way around. But I think it's clearer the first way and it's fine in this case. If you do % and set the stepping to 10% it will do 10% to 100% circles. There is a tiny chance it will miss the 100% circle. But I tried the existing program and it worked fine. Plus it's no big deal if the 100% circle isn't printed. People know where 100% is (it's the outer edge of the ronchi/foucault).
And for other cases such as stepping by 10mm or stepping by 1 inch etc, it's unlikely to ever end up exactly at the outer edge anyway.
Also my suggested code now has a small bug because we have rounded (or "floored") the step size to the nearest integer so it will understep in many cases. So really it's buggier if we use an integer in the loop.
So @atsju, is there some comment we can add to this line of code to tell it not to worry about a double as a loop counter?
There was a problem hiding this comment.
To be clear, I'm leaning to agree with clang on that other complaint of a double in a loop, the one where it went from -1 to 1. I'm going to look at that code probably in the next few days. But in this case I think the double is the better way to do this code.
|
🚀 New build available for commit |
|
I think the float is just fine. The comparison is not "==" . Yes I know "==" is a problem and why. But this compare is a "<=". I did not create that code the AI did. However if I was writing it I probably would have done the same. |
|
To clarify again. I'm fine with the clang warning that appears in comment section of they are about float comparison as long as you have a small look at it. This is OK. I will check how to remove them from comment to keep warning only in review pane + how to mute the warnings for intentional an verified cases like here. However, I would like the compilation warning that are only seen in review tab to be fixed. |
|
Oh! I think this image shows all the warnings nicely. There are 3 connects with lambda functions in them as shown here in QT: Fixing the "context" warning is trivial - just insert "this" as a new 3rd parameter and leave the rest as is. Regarding the other warnings, I thought about this for a while and I think it's okay as is. The 3 connects are acting on actions of 3 buttons that can be clicked and then they call a lambda function passing all member variables many of which are referenced and may not exist if a user closes the window. But you can't click on a button after the window is gone so it should be fine. HOWEVER So 5 lines of code change. But really I should probably also refactor "colors" to "m_colors_temp" or something like that but that means like 15 lines of code change. What do you guys think? Here's my change that works: 
and in foucault.h 
|
|
My opinion. A lot of work for no reason caused by an inaccurate warning that cannot understand the code. |
|
In my opinion, some warnings have proven to be usefull. Even if this one is false positive, it's better to write code that does not trigger warning. It also makes the code more maintainable as next developper will not need to re-analyse the context to understand if the code he sees "works in this specific case". |
|
Dale is it okay if I just go ahead and push it? At this point it will just take me seconds. I already tested it. Also should I rename "colors" to m_colors_temp" or something similar? |
|
prepending with m_ is supposed to mean it is a member variable. In this case it is not a member variable it is a local variable so no do not prepend with a m_ Yes go ahead. What a waist of time. |
|
The issue is not that some warnings are valid. The issue is too many false positives. That perhaps takes more time to solve than finding the bugs manually and confuse future novice coders. |
|
The solution (shown a few posts ago in that red and green text) turns that variable into a member variable. Take a look again above at the one green line added to the .h file. That's a member variable. It's a temporary variable only used while the dialog is up to store the2 colors in case the user hits "ok" (instead of cancel). Storing the variable as a local variable doesn't work - I don't know how to do that with lambda functions and making clangd happy. |
|
Use the = sign or add it to the local parameter list in the lambda. If neither of those work then the code checker should not be used. |
|
I tried "=" and even named storage explicity but clangd doesn't like it because it's afraid "storage" will go out of scope when the lambda function is called. So...
This is a good point of course. But clangd already found a bug or two. Plus I think it might have found a bug in your double loop going from -1 to +1 for the profile but I haven't looked at that carefully yet. So the bug may be unrelated. The question is are the false positives more work than the time saved fixing bugs before we know about them. If you don't like my changes to suppress the warnings just let me know and we can skip them. |
|
In my opinion yes the warnings are more trouble than they are worth. Yes go ahead since you have already done it. We have already spent too much time on nothing. |
closes #332