[Solved = False Alarm] Exit from within criticalsection? (application.inc)

[ASerge]

The flow will be clearer.

Do you really think that code with an additional variable that still needs to be monitored becomes cleaner than this:

Code: Pascal  [Select][+][-]

1. ...
2.     repeat
3.       // remove top item from queue
4.       System.EnterCriticalSection(CritSec);
5.       try
6.         if Cur.Top <> nil then
7.           Exit;
8.       finally
9.         System.LeaveCriticalSection(CritSec);
10.       end;
11.     until OtherConds;      
12. ...

[Warfley]

The flow will be clearer.

No it's not, first the most obvious: Translating a for loop into a while loop is actually more difficult than it may seem as they evaluate the loop bounds only once and also handle overflows correctly. So in general a for loop like this:

Code: Pascal  [Select][+][-]

1. for i := Start to Stop do
2.   ...
3.  

Translated to a while loop is:

Code: Pascal  [Select][+][-]

1. i:=Start;
2. loopEnd:=End;
3. StopCondition:=i<=LoopEnd;
4. while not StopCondition do
5. begin
6.   ...
7.   if i=LoopEnd then
8.     StopCondition:=True
9.   else
10.     Inc(i);
11. end;

And as your approach solely works with while or repeat loops, this makes any for loop an unreadable mess.

The second problem is state explosion. Every new variable you introduce lets the possible states exponentially grow. This does not only make it in theory harder to grasp what is happening there, it also makes certain static analysis tools much more difficult. Take the following code:

Code: Pascal  [Select][+][-]

1. if SomeCondition then
2. begin
3.   CanContinue:=True;
4.   MyObject := TSomeClass.Create;
5. end
6. else ...
7. if CanContinue then
8.   MyObject.Free;

Very simple code right? When you compile this with -Oodfa it will throw a warning that MyObject is potentially not assigned because it cannot make the inference over the CanContinue variable. If you compare this to:

Code: Pascal  [Select][+][-]

1. if SomeCondition then
2. begin
3.   MyObject := TSomeClass.Create;
4. end
5. else
6. begin
7.   ...
8.   break;
9. end;
10. MyObject.Free;

The DFA works as expected and recognizes that MyObject will always be assigned. This is because following control flow is much simpler than evaluating boolean expressions

[Xenno]

@ASerge 
You’re right — it is cleaner and shorter, but I don’t find it clearer. That's the common reason I mentioned. I’ve tried to draw their flowcharts for comparison, and I’d appreciate your advice on how to correctly draw the second chart.

@Warfley 
Please excuse my limited knowledge, but I don’t fully understand your point. There is no loop translation and no “Create-Free” object in this context.


To summarize: Exit, Break, and Continue are system procedures with flow-control capabilities that can be used at any time. Personally, I prefer to avoid them, as it makes the process flow easier to understand. It’s like jumping out of a bus window instead of using the door — possible, but not the clearest way. 

I apologize if my English limits my ability to argue further, but I hope this clarifies my perspective.

Cheers,

[Warfley]

@Warfley 
Please excuse my limited knowledge, but I don’t fully understand your point. There is no loop translation and no “Create-Free” object in this context.

The create and Free is just an example. My point is that if your control flow is dependent on boolean switches, you make the Data Flow Analysis very unhappy. It is really good at finding unused variables or read before write errors, if you are restricted to simple control flow, but really suffers from state management (through switches on variables, etc.).
Assuming you have the following code:

Code: Pascal  [Select][+][-]

1. var
2.   i, x: Integer;
3. begin
4.   i:=0;
5.   while i < 10 do
6.   begin
7.     if Cond1 then
8.       x:=0
9.     else if Cond2 then
10.       x:=1
11.     else
12.       break;
13.     WriteLn(x);
14.     i := i + 2;
15.   end;
16. end.

If I use the code pattern you have shown it would become:

Code: Pascal  [Select][+][-]

1. var
2.   i, x: Integer;
3.   didBreak: Boolean;
4. begin
5.   i:=0;              
6.   didBreak:=False;
7.   while not DidBreak and (i < 10) do
8.   begin
9.     if Cond1 then
10.       x:=0
11.     else if Cond2 then
12.       x:=1
13.     else
14.       didBreak:=True;
15.     // If break was called don't execute the rest
16.     if not didBreak then
17.     begin
18.       WriteLn(x);
19.       i := i + 2;
20.     end;
21.   end;
22. end.

If I compile the first version with -Oodfa to enable data flow analysis everything is fine. If I compile the second example, I get a warning at the "WriteLn(x);":  project1.lpr(32,16) Warning: Variable "x" does not seem to be initialized

As warnings are just errors waiting to happen (and I generally recommend compiling with -Sew switch to treat warnings as errors) you want to avoid something like this.

[Xenno]

@Warfley
The second code example is problematic because x is never initialized. It’s always good practice to assign an initial value to a variable, especially if its value will later be read. Without initialization, the logic becomes unclear and potentially unsafe.

Since I don’t use VB’s ElseIf style, please allow me to reformat the code for clarity.

Code: Pascal  [Select][+][-]

1. var
2.   i, x: Integer;
3.   didBreak: Boolean;
4. begin
5.   i := 0;              
6.   didBreak := False;
7.   while not(didBreak) and (i < 10) do
8.   begin
9.     if Cond1 then
10.       x := 0
11.     else
12.       if Cond2 then
13.         x := 1
14.       else
15.         didBreak := True;
16.     // If break was called don't execute the rest
17.     if not(didBreak) then
18.     begin
19.       WriteLn(x);
20.       i := i + 2;
21.     end;
22.   end;
23. end.

Now it’s obvious that x starts without a value. To fix this, initialize x somewhere before calling writeln(x), as long as doing so doesn’t break the intended logic. Even in your first code, initializing variables is a habit worth keeping.

Try creating flowcharts for both versions of the code. Which one is easier to follow? Flowcharts often reveal readability.

As for Exit, Break, and Continue: they’re not forbidden, and compiler won’t warn you about them. They can make your process flow more artistic so use them carefully. Overuse or misuse can lead to unexpected outcomes.

[Warfley]

Now it’s obvious that x starts without a value. To fix this, initialize x somewhere before calling writeln(x), as long as doing so doesn’t break the intended logic. Even in your first code, initializing variables is a habit worth keeping.

Initialize it with what value? This is not a "fix". It is either 0 if Cond1 is True or 1 if Con2 is true. Neither Cond1 nor Cond2 is true it's value is undefined. If that is the semantic than any initialization does not make sense. If I Initialize it with 0 then if I have a bug that skips the cond1 check, it would still look like Cond1 was true, same for 1 with Cond2. If I initialize it to something like -1 it is an undefined value so if I neither go into Cond1 nor Cond2 branch and somehow end up later it is a bug.

So by initializing the Value, I just mask a bug. If I don't initialize the value and use the DFA the DFA tells me I have bug and I can fix it.
Initializing the variable to make the DFA shut up is just masking potential bugs. Thats exactly the opposite of what you should do.
The goal is not to not have any warnings, the goal is to have correct code and build your code in a way that the compiler tells you (through warnings) if something is wrong.

Try creating flowcharts for both versions of the code. Which one is easier to follow? Flowcharts often reveal readability.

I don't like flowcharts, they just add a layer of potential bugs. If I create a flow chart of some code and I change the code but forget to update the flow chart, anyone reading the flow chart will make wrong assumptions over the code.

What I want instead is good tooling that ensures if my code has mistakes it notifies me. So the goal is to write easily verifiable by tooling

[Xenno]

Any value will do. What bug? After adding initization that code will be very good. I don't see x will be read after the loop.

[Warfley]

Any value will do.

And whats the meaning of the value? If you just initialize with a value so it has a value, whats the point? Like what is the semantic of the initialization? There are just some times where there is no such thing as a meaningful value. E.g. take the following:

Code: Pascal  [Select][+][-]

1. if isNumeric(someString) then
2.   x := StrToInt(someString)
3. else if isFloat(someString) then
4.   x := Round(StrToFloat(someString));
5. else
6.   break;

If SomeString is neither an integer nor a float, what value is x? The only correct answer is None, there is no value for x because x can only have a value if either of those two cases is true. If I set this to 0, there is no way to distinguish if the value 0 was read from someString or it was the initial value.
For example consider for debugging purposes I commented out the break, and then forget to undo that, things like that happen all the time. In that case the DFA would tell me: Hey you are having a path where x is not initialized, and I check my code, find my mistake and am happy. If I initialize x, the DFA will not show that warning and I keep wondering why things go wrong.

Trying to avoid warnings instead of avoiding the reasons warnings occur, is a very bad habit

Note that some languages like Typescript have a bottom type (undefined in typescript), which allows to explicitly undefine the variable and have the typechecker validate that whenever you access the value it's defined. Pascal doesn't have that, so using initialization is the only thing we have to get similar guarantees

[Xenno]

And whats the meaning of the value? If you just initialize with a value so it has a value, whats the point? Like what is the semantic of the initialization? There are just some times where there is no such thing as a meaningful value.

To let compiler be sure we won't read uninitialized variable. Later on, the value correctness relies on the code logic Even in that code x is global, according to you, compiler still shows warning. The compiler is working awesome. Then when Break exists, as you said, this handy alert is muted. Because you are sure the value might not be read. Since writeln(x) appears after Break, maybe compiler will be just less strict. I don't know, my noviceness does not reach that level. What if you code is longer? It is a short word and easy to miss. Can we see a risk?

For example consider for debugging purposes I commented out the break, and then forget to undo that, things like that happen all the time. In that case the DFA would tell me: Hey you are having a path where x is not initialized, and I check my code, find my mistake and am happy. If I initialize x, the DFA will not show that warning and I keep wondering why things go wrong.

The compiler gets its power back after Break unseen.

Trying to avoid warnings instead of avoiding the reasons warnings occur, is a very bad habit

Is it related to intializing variables? Well...

Note that some languages like Typescript have a bottom type (undefined in typescript), which allows to explicitly undefine the variable and have the typechecker validate that whenever you access the value it's defined. Pascal doesn't have that, so using initialization is the only thing we have to get similar guarantees

Very agree.

https://www.freepascal.org/docs-html/ref/refse24.html
https://docs.freepascal.org/docs-html/current/ref/refse25.html

[Warfley]

To let compiler be sure we won't read uninitialized variable.

Yeah but if I access a wrongly initialized variable I still have a bug, just no DFA to tell me. I rather have the a bug the compiler tells me about than a bug the compiler doesn't. I only initialize variables when it makes semantic sense, when there is a default value that has some purpose. Everything else I just replace no initialization with a faulty one, which is actually worse.

Then when Break exists, as you said, this handy alert is muted. Because you are sure the value might not be read. Since writeln(x) appears after Break, maybe compiler will be just less strict. I don't know, my noviceness does not reach that level. What if you code is longer?

The compiler uses a rather simple data flow analysis. Basically it goes through the code backwards and checks for each read if there is any theoretical path through the code where the variable was not written. Theoretical because it does not assess logic, meaning the following:

Code: Pascal  [Select][+][-]

1. if x < 0 then
2.   y := 10;
3. if x >= 0 then
4.   y := 20;

while to any human it is obious that there is no logical way through the code that y is unitialized, to the DFA it isn't. This is a complexity problem as this requires SMT/SAT solving which is very difficult (even though solvers like Z3 can be in many cases quite efficient). So, as the FPC does not have an SMT solver included (I think clang for C++ already does for optimization purposes), the DFA analysis of the FPC basically just takes the control flow as written in language and assumes that for any if also the else branch is possible, for any while the condition can be both true and false, and for any loop the loop may have 0 or more iterations. But it correctly handles exit, break, continue and I think even goto and noreturn functions.

There are cases where you cannot avoid having some logic based access controls. For example sometimes you have cases where you need to create a temporary object and later if that temporary object was created you need to do cleanup. Then you need to initialize the variable to make the DFA shut up, but that is more the exception than the rule. Most well written code the DFA will work fine and I have found a bunch of bugs because of it before even running the software. But only if you don't make it purposefully harder than it should be. This is why I would always prefer to use break/continue/exit over switch variables.

Additionally I personally find switch variables make the code much harder to understand, as you don't think that way, there is no reason to argue that, I just wanted to bring up the point of the DFA which can make a meaningful difference

[Xenno]

Thank you very much for the explanation. That's definitely beyond my knowledge.