Code clean up at TApplication.DoBeforeFinalization

[lagprogramming]

lcl/include/application.inc has the following procedure:

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

1. procedure TApplication.DoBeforeFinalization;
2. var
3.   i: Integer;
4. begin
5.   if Self=nil then exit;
6.   for i := ComponentCount - 1 downto 0 do
7.   begin
8.     // DebugLn('TApplication.DoBeforeFinalization ',DbgSName(Components[i]));
9.     if i < ComponentCount then
10.       Components[i].Free;
11.   end;
12. end;

The line if i < ComponentCount then is useless. Variable i will always be smaller than ComponentCount.
The following patch removes it.

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

1. diff --git a/lcl/include/application.inc b/lcl/include/application.inc
2. index e2ce3b6988..2587588996 100644
3. --- a/lcl/include/application.inc
4. +++ b/lcl/include/application.inc
5. @@ -1083,8 +1083,7 @@ begin
6.    for i := ComponentCount - 1 downto 0 do
7.    begin
8.      // DebugLn('TApplication.DoBeforeFinalization ',DbgSName(Components[i]));
9. -    if i < ComponentCount then
10. -      Components[i].Free;
11. +    Components[i].Free;
12.    end;
13.  end;

[AlexTP]

Posted to https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/40393

[runewalsh]

I’m sure this check is for a reason, namely, a component can remove several others in its destructor. I sometimes use this idiom (iteration backwards w/recheck agains count) exactly for this reason.

[lagprogramming]

I’m sure this check is for a reason, namely, a component can remove several others in its destructor. I sometimes use this idiom (iteration backwards w/recheck agains count) exactly for this reason.

Indeed, there is a reason. It sholdn't be removed, but something has to be done.
procedure TApplication.DoIdleActions can be found in the same file and has the following lines:

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

1.   i := Screen.CustomFormCount-1;
2.   while i >=0 do begin { While loop to allow number of forms to change during loop }
3.     CurForm:=Screen.CustomForms[i];
4.     if CurForm.FormStyle=fsSplash then
5.       CurForm.Hide;
6.     i:=Min(i,Screen.CustomFormCount)-1;
7.   end;

Also, in the same file is procedure TApplication.IconChanged, procedure that has the following lines.

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

1.   i := Screen.CustomFormCount-1;
2.   while i >=0 do begin { While loop to allow number of forms to change during loop }
3.     CurForm:=Screen.CustomForms[i];
4.     CurForm.Perform(CM_ICONCHANGED, 0, 0);
5.     i:=Min(i,Screen.CustomFormCount)-1;
6.   end;

In addition to the lines i:=Min(i,Screen.CustomFormCount)-1;, there is a useful comment writing that that the number of forms is allowed to change. It's easy to read and understand that code.
After reading three comments in that file that a while loop is used instead of a for loop because the number of whatever is allowed to change during loop you read the code below.

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

1. procedure TApplication.DoBeforeFinalization;
2. var
3.   i: Integer;
4. begin
5.   if Self=nil then exit;
6.   for i := ComponentCount - 1 downto 0 do
7.   begin
8.     // DebugLn('TApplication.DoBeforeFinalization ',DbgSName(Components[i]));
9.     if i < ComponentCount then
10.       Components[i].Free;
11.   end;
12. end;

Obviously you would consider something is wrong. I think the code of the routines should look alike, be similar, but which code would the lcl developers prefer? The code of TApplication.DoBeforeFinalization needs at least an additional comment.
The equivalent of TApplication.DoIdleActions and TApplication.IconChanged would be

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

1. procedure TApplication.DoBeforeFinalization;
2. var
3.   i: Integer;
4. begin
5.   if Self=nil then exit;
6.   i:=ComponentCount-1;
7.   while i>=0 do begin { While loop to allow number of components to change during loop }
8.     // DebugLn('TApplication.DoBeforeFinalization ',DbgSName(Components[i]));
9.     Components[i].Free;
10.     i:=Min(i,ComponentCount)-1;
11.   end;
12. end;

[runewalsh]

I think while version is a lot less readable (and probably larger and slower), the maximum that the original code requires is a comment, but even that is redundant for a REASONABLY EXPERIENCED READER.

if Self=nil then exit; part is more suspicious btw.

[Josh]

does the code below work

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

1. procedure TApplication.DoBeforeFinalization;
2. Begin
3.   if Self=nil then exit;
4.   If ComponentCount >0 Then
5.   Begin
6.     Repeat
7.       Components[ComponentCount -1].free;
8.     Until ComponentCount =0;
9.   End;
10. End;
11.  

[TRon]

@Josh:
Isn't that exactly the same as:

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

1. while ComponentCount > 0
2.   do Components[ComponentCount-1].free
3.  

[Josh]

Hi Tron,

Yes its the same..

Odd I thought I edited my post and added similar in last night before bed. A Repeat vs While. Should check better.

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

1. procedure TApplication.DoBeforeFinalization;
2. Begin
3.   if Self=nil then exit;
4.   While ComponentCount >0 do
5.   Begin
6.     Components[ComponentCount -1].free;
7.   End;
8. End;

To make similar to original adding the debug line in

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

1. procedure TApplication.DoBeforeFinalization;
2. Begin
3.   if Self=nil then exit;
4.   If ComponentCount >0 Then
5.   Begin
6.     Repeat
7.       // DebugLn('TApplication.DoBeforeFinalization ',DbgSName(Components[ComponentCount -1]));
8.       Components[ComponentCount -1].free;
9.     Until ComponentCount =0;
10.   End;
11. End;
12.  
13. procedure TApplication.DoBeforeFinalization;
14. Begin
15.   if Self=nil then exit;
16.   While ComponentCount >0 do
17.   Begin
18.     // DebugLn('TApplication.DoBeforeFinalization ',DbgSName(Components[ComponentCount -1]));
19.     Components[ComponentCount -1].free;
20.   End;
21. End;

[Thaddy]

Of course it should be for High()... downto, silly buggers...
It is always better and likely generates better code if free is called in the reverse order of creation.

[lagprogramming]

I think while version is a lot less readable (and probably larger and slower), the maximum that the original code requires is a comment, but even that is redundant for a REASONABLY EXPERIENCED READER.

if Self=nil then exit; part is more suspicious btw.

I've searched for if self in the lazarus directory, looking for nil values checks and lcl/include.application.inc has four if self=nil then occurrences.
Regarding the loops, by using for i:=...downto... or i:=Min(i,)-1, the original code found in the mentioned three routines guarantees that the loop won't run forever. Maybe it's a workaround for the situation when components are also endlessly created. The question is, what happens with the components created during the loop.