Code clean up at TApplication.DoBeforeFinalization
lazarus / 7-LCL

----------------------------------------------------------------------
lagprogramming · 2023-07-21 09:34

lcl/include/application.inc has the following procedure:
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TApplication.DoBeforeFinalization;var  i: Integer;begin  if Self=nil
then exit;  for i := ComponentCount - 1 downto 0 do   begin    //
DebugLn('TApplication.DoBeforeFinalization
',DbgSName(Components[i]));    if i < ComponentCount then      
Components[i].Free;  end;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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i diff --git
a/lcl/include/application.inc b/lcl/include/application.incindex
e2ce3b6988..2587588996 100644--- a/lcl/include/application.inc+++
b/lcl/include/application.inc@@ -1083,8 +1083,7 @@ begin   for i :=
ComponentCount - 1 downto 0 do    begin     //
DebugLn('TApplication.DoBeforeFinalization
',DbgSName(Components[i]));-    if i < ComponentCount then -    
 Components[i].Free;+    Components[i].Free;   end; end;

----------------------------------------------------------------------
AlexTP · 2023-07-21 11:06

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

----------------------------------------------------------------------
runewalsh · 2023-07-21 11:11

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 · 2023-07-21 14:42

Quote from: runewalsh on July 21, 2023, 01:11:05 pmI’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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i   i :=
Screen.CustomFormCount-1;  while i >=0 do begin { While loop to allow
number of forms to change during loop }   
CurForm:=Screen.CustomForms[i];    if CurForm.FormStyle=fsSplash then 
    CurForm.Hide;    i:=Min(i,Screen.CustomFormCount)-1;  end;Also, in
the same file is procedure TApplication.IconChanged, procedure that
has the following lines.
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i   i :=
Screen.CustomFormCount-1;  while i >=0 do begin { While loop to allow
number of forms to change during loop }   
CurForm:=Screen.CustomForms[i];    CurForm.Perform(CM_ICONCHANGED, 0,
0);    i:=Min(i,Screen.CustomFormCount)-1;  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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TApplication.DoBeforeFinalization;var  i: Integer;begin  if Self=nil
then exit;  for i := ComponentCount - 1 downto 0 do   begin    //
DebugLn('TApplication.DoBeforeFinalization
',DbgSName(Components[i]));    if i < ComponentCount then      
Components[i].Free;  end;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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TApplication.DoBeforeFinalization;var  i: Integer;begin  if Self=nil
then exit;  i:=ComponentCount-1;  while i>=0 do begin { While loop to
allow number of components to change during loop }    //
DebugLn('TApplication.DoBeforeFinalization
',DbgSName(Components[i]));    Components[i].Free;   
i:=Min(i,ComponentCount)-1;  end;end;

----------------------------------------------------------------------
runewalsh · 2023-07-21 16:35

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 · 2023-07-22 01:09

does the code below work

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TApplication.DoBeforeFinalization;Begin  if Self=nil then exit;  If
ComponentCount >0 Then  Begin    Repeat      Components[ComponentCount
-1].free;    Until ComponentCount =0;  End;End;

----------------------------------------------------------------------
TRon · 2023-07-22 06:41

@Josh:
Isn't that exactly the same as:
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i while
ComponentCount > 0  do Components[ComponentCount-1].free

----------------------------------------------------------------------
Josh · 2023-07-22 08:50

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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TApplication.DoBeforeFinalization;Begin  if Self=nil then exit;  While
ComponentCount >0 do  Begin    Components[ComponentCount -1].free; 
End;End;
To make similar to original adding the debug line in
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TApplication.DoBeforeFinalization;Begin  if Self=nil then exit;  If
ComponentCount >0 Then  Begin    Repeat      //
DebugLn('TApplication.DoBeforeFinalization
',DbgSName(Components[ComponentCount -1]));     
Components[ComponentCount -1].free;    Until ComponentCount =0; 
End;End; procedure TApplication.DoBeforeFinalization;Begin  if
Self=nil then exit;  While ComponentCount >0 do  Begin    //
DebugLn('TApplication.DoBeforeFinalization
',DbgSName(Components[ComponentCount -1]));   
Components[ComponentCount -1].free;  End;End;

----------------------------------------------------------------------
Thaddy · 2023-07-22 19:02

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 · 2023-07-23 06:58

Quote from: runewalsh on July 21, 2023, 06:35:06 pmI 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.