[Sketcher] Possible bug in new line split code

[chennes]

LGTM caught what appears to be a logic error on the line splitting code added by git commit 4d6b1f3. It takes the form of code that looks like this:

Code: Select all

do {
            ...
            int newId = addGeometry(newLine);
            if (newId < 0) {
                continue;
            }
           ...
} while (false);

I don't know what's intended here, exactly -- LGTM complains because that continue statement does not do what programmers normally think of "continue" as meaning, which is to restart the loop. Because the loop condition is always false, this continue actually functions as a break. Is that the intended behavior? Or is this code expecting to restart the loop from the top?

(cppreference has a good explanation of how continue works)

[openBrain]

I think the behavior is correct (looking at a glance) but it's indeed weird and 'continue' would happily be replaced by 'break'.

[tpavlicek]

Hello,

the intention here is to stop any further processing if adding a new edge failed. I agree, the whole construction

Code: Select all

do {
   ...
   if (...) continue;
   ...
}
while (false);

is rather unorthodox, but goto could be considered even worse ;-) Of course, I could have replaced this by try/catch, but I am rather plain C guy, so if it is not neccessary, I stick to basic statements.

If continue raises false positives here, please feel free to replace it with break.

Kind regards,


Tomas

[openBrain]

Of course, I could have replaced this by try/catch, but I am rather plain C guy, so if it is not neccessary, I stick to basic statements.

I'm also basically a C coder.
But even there, 'break' better denotes the intention IMO.
'break' is "unconditionally exit the current loop without further processing".
'continue' is "re-evaluate conditional statement of the loop and proceed accordingly".
Anyway, thanks for fast feedback.

[chennes]

So if I am understanding this code correctly, then, it's not ever intended to loop at all, is that correct? It's essentially just mimicking goto behavior without making explicit the goto itself?

ETA: Here's a PR with my proposed change... https://github.com/FreeCAD/FreeCAD/pull/4990