[SOLVED] Improvement of procedure SysGetEnvironmentList

[lagprogramming]

packages/fcl-base/src/custapp.pp has the following procedure:

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

1. Procedure SysGetEnvironmentList(List : TStrings;NamesOnly : Boolean);
2.  
3. var
4.    s : string;
5.    i,l,j,count : longint;
6.  
7. begin
8.   count:=GetEnvironmentVariableCount;
9.   if count>0 then
10.     for j:=1 to count  do
11.      begin
12.        s:=GetEnvironmentString(j);
13.        l:=Length(s);
14.        If NamesOnly then
15.           begin
16.             I:=pos('=',s);
17.             If (I>0) then
18.               S:=Copy(S,1,I-1);
19.           end;
20.        List.Add(S);
21.     end;
22. end;

Variable l is declared and a value is assigned to it but it's never used.
Also, the line if count>0 then adds an unnecessary conditional jump. Either it remains and the for loop is replaced with a repeat loop, either the if statement is removed.

1/2 The following patch removes the l variable declaration, the l:=Length(s); line and the if count>0 then line.

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

1. diff --git a/packages/fcl-base/src/custapp.pp b/packages/fcl-base/src/custapp.pp
2. index adb0ff46ae..c6a4b064b8 100644
3. --- a/packages/fcl-base/src/custapp.pp
4. +++ b/packages/fcl-base/src/custapp.pp
5. @@ -176,22 +176,20 @@ function TCustomApplication.GetExeName: string;
6.  
7.  var
8.     s : string;
9. -   i,l,j,count : longint;
10. +   i,j,count : longint;
11.  
12.  begin
13.    count:=GetEnvironmentVariableCount;
14. -  if count>0 then
15. -    for j:=1 to count  do
16. -     begin
17. -       s:=GetEnvironmentString(j);
18. -       l:=Length(s);
19. -       If NamesOnly then
20. -          begin
21. -            I:=pos('=',s);
22. -            If (I>0) then
23. -              S:=Copy(S,1,I-1);
24. -          end;
25. -       List.Add(S);
26. +  for j:=1 to count  do
27. +    begin
28. +    s:=GetEnvironmentString(j);
29. +    If NamesOnly then
30. +      begin
31. +      I:=pos('=',s);
32. +      If (I>0) then
33. +        S:=Copy(S,1,I-1);
34. +      end;
35. +    List.Add(S);
36.      end;
37.  end;

2/2 One last thing. Unlike the SysGetEnvironmentList functions in packages/fcl-base/src/go32v2/custapp.inc and packages/fcl-base/src/os2/custapp.inc which contain the code
If (I>1) then S:=Copy(S,1,I-1);
the SysGetEnvironmentList function in fcl-base/src/custapp.pp contains the code
If (I>0) then S:=Copy(S,1,I-1);
I was thinking that maybe they should look alike, I mean compare I with the same value.

[AlexTP]

Posted to https://gitlab.com/freepascal.org/fpc/source/-/issues/40347

[Josh]

no knowing how the diff works, but curious as why the mod has line

@@ -176,22 +176,20 @@ function TCustomApplication.GetExeName: string;

does it means it for function GetExeName rather than procedure SysGetEnvironmentList ?

I agree keep the for loop rather than a repeat or while loop, if memory serves its faster and can optimize better.

you could remove the count var too

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

1. Procedure SysGetEnvironmentList(List : TStrings;NamesOnly : Boolean);
2. var
3.   s : string;
4.   i,j : longint;
5. begin
6.   for j:=1 to GetEnvironmentVariableCount  do
7.   Begin
8.     s:=GetEnvironmentString(j);
9.     If NamesOnly then
10.     begin
11.       i:=pos('=',s);
12.       If (i>0) then S:=Copy(S,1,i-1);
13.     end;
14.     List.Add(S);
15.   end;
16. end;

[lagprogramming]

no knowing how the diff works, but curious as why the mod has line

@@ -176,22 +176,20 @@ function TCustomApplication.GetExeName: string;

does it means it for function GetExeName rather than procedure SysGetEnvironmentList ?

Indeed, reading that text line adds confusion or misleading. It's not what we might expect from it.