View Issue Details

IDProjectCategoryView StatusLast Update
0037782PatchesPackagespublic2020-10-11 15:03
ReporterCudaText man_ Assigned ToBart Broersma  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
OSubuntu x64 
Product Version2.1 (SVN) 
Target Version2.2Fixed in Version2.2 
Summary0037782: LazUtils.FileUtil.FindAllFiles patch
Description  - added 'const' for few string params
  - added value for Searcher.Search parameter "CaseSensitive" (was False) as FilenamesCaseSensitive
  (const in FileUtil). This will make search faster on linux.
TagsNo tags attached.
Fixed in Revisionr63989
LazTarget2.2
Widgetset
Attached Files

Activities

CudaText man_

2020-09-21 01:02

reporter  

find.diff (2,259 bytes)   
Index: components/lazutils/fileutil.inc
===================================================================
--- components/lazutils/fileutil.inc	(revision 63900)
+++ components/lazutils/fileutil.inc	(working copy)
@@ -656,23 +656,23 @@
 end;
 
 procedure FindAllFiles(AList: TStrings; const SearchPath: String;
-  SearchMask: String; SearchSubDirs: Boolean; DirAttr: Word;
+  const SearchMask: String; SearchSubDirs: Boolean; DirAttr: Word;
   MaskSeparator: char; PathSeparator: char);
 var
   Searcher: TListFileSearcher;
 begin
   Searcher := TListFileSearcher.Create(AList);
-  Searcher.DirectoryAttribute := DirAttr;
-  Searcher.MaskSeparator := MaskSeparator;
-  Searcher.PathSeparator := PathSeparator;
   try
-    Searcher.Search(SearchPath, SearchMask, SearchSubDirs);
+    Searcher.DirectoryAttribute := DirAttr;
+    Searcher.MaskSeparator := MaskSeparator;
+    Searcher.PathSeparator := PathSeparator;
+    Searcher.Search(SearchPath, SearchMask, SearchSubDirs, FilenamesCaseSensitive);
   finally
     Searcher.Free;
   end;
 end;
 
-function FindAllFiles(const SearchPath: String; SearchMask: String;
+function FindAllFiles(const SearchPath: String; const SearchMask: String;
   SearchSubDirs: Boolean; DirAttr: Word;
   MaskSeparator: char; PathSeparator: char): TStringList;
 begin
Index: components/lazutils/fileutil.pas
===================================================================
--- components/lazutils/fileutil.pas	(revision 63900)
+++ components/lazutils/fileutil.pas	(working copy)
@@ -180,11 +180,11 @@
     constructor Create(AList: TStrings);
   end;
 
-function FindAllFiles(const SearchPath: String; SearchMask: String = '';
+function FindAllFiles(const SearchPath: String; const SearchMask: String = '';
   SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
   MaskSeparator: char = ';'; PathSeparator: char = ';'): TStringList; overload;
 procedure FindAllFiles(AList: TStrings; const SearchPath: String;
-  SearchMask: String = ''; SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
+  const SearchMask: String = ''; SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
   MaskSeparator: char = ';'; PathSeparator: char = ';'); overload;
 
 function FindAllDirectories(const SearchPath: string;
find.diff (2,259 bytes)   

CudaText man_

2020-09-21 01:33

reporter   ~0125692

Optimized TFileSearcher.Search, avoided temp string changing.
find2.diff.
find2.diff (3,798 bytes)   
Index: components/lazutils/fileutil.inc
===================================================================
--- components/lazutils/fileutil.inc	(revision 63900)
+++ components/lazutils/fileutil.inc	(working copy)
@@ -656,23 +656,23 @@
 end;
 
 procedure FindAllFiles(AList: TStrings; const SearchPath: String;
-  SearchMask: String; SearchSubDirs: Boolean; DirAttr: Word;
+  const SearchMask: String; SearchSubDirs: Boolean; DirAttr: Word;
   MaskSeparator: char; PathSeparator: char);
 var
   Searcher: TListFileSearcher;
 begin
   Searcher := TListFileSearcher.Create(AList);
-  Searcher.DirectoryAttribute := DirAttr;
-  Searcher.MaskSeparator := MaskSeparator;
-  Searcher.PathSeparator := PathSeparator;
   try
-    Searcher.Search(SearchPath, SearchMask, SearchSubDirs);
+    Searcher.DirectoryAttribute := DirAttr;
+    Searcher.MaskSeparator := MaskSeparator;
+    Searcher.PathSeparator := PathSeparator;
+    Searcher.Search(SearchPath, SearchMask, SearchSubDirs, FilenamesCaseSensitive);
   finally
     Searcher.Free;
   end;
 end;
 
-function FindAllFiles(const SearchPath: String; SearchMask: String;
+function FindAllFiles(const SearchPath: String; const SearchMask: String;
   SearchSubDirs: Boolean; DirAttr: Word;
   MaskSeparator: char; PathSeparator: char): TStringList;
 begin
@@ -765,7 +765,7 @@
   FSearching := False;
 end;
 
-procedure TFileSearcher.Search(ASearchPath: String; ASearchMask: String;
+procedure TFileSearcher.Search(const ASearchPath: String; const ASearchMask: String;
   ASearchSubDirs: Boolean; CaseSensitive: Boolean = False);
 var
   MaskList: TMaskList;
@@ -836,9 +836,9 @@
   end;
 
 var
-  p: SizeInt;
+  p1, p2: Integer;
+  i: Integer;
   Dir: String;
-  i: Integer;
   OtherDir: String;
   MaskOptions: TMaskOptions;
 begin
@@ -855,12 +855,14 @@
   FSearching := True;
   SearchDirectories:=TStringList.Create;
   try
-    while ASearchPath<>'' do begin
-      p:=Pos(FPathSeparator,ASearchPath);
-      if p<1 then
-        p:=length(ASearchPath)+1;
-      Dir:=ResolveDots(LeftStr(ASearchPath,p-1));
-      Delete(ASearchPath,1,p);
+    p1:=1;
+    while p1<=Length(ASearchPath) do
+    begin
+      p2:=PosEx(FPathSeparator,ASearchPath,p1);
+      if p2<1 then
+        p2:=length(ASearchPath)+1;
+      Dir:=ResolveDots(Copy(ASearchPath,p1,p2-p1));
+      p1:=p2+1;
       if Dir='' then continue;
       Dir:=ChompPathDelim(Dir);
       for i:=SearchDirectories.Count-1 downto 0 do
Index: components/lazutils/fileutil.pas
===================================================================
--- components/lazutils/fileutil.pas	(revision 63900)
+++ components/lazutils/fileutil.pas	(working copy)
@@ -145,7 +145,7 @@
     procedure DoFileFound; virtual;
   public
     constructor Create;
-    procedure Search(ASearchPath: String; ASearchMask: String = '';
+    procedure Search(const ASearchPath: String; const ASearchMask: String = '';
       ASearchSubDirs: Boolean = True; CaseSensitive: Boolean = False);
   public
     property MaskSeparator: char read FMaskSeparator write FMaskSeparator;
@@ -180,11 +180,11 @@
     constructor Create(AList: TStrings);
   end;
 
-function FindAllFiles(const SearchPath: String; SearchMask: String = '';
+function FindAllFiles(const SearchPath: String; const SearchMask: String = '';
   SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
   MaskSeparator: char = ';'; PathSeparator: char = ';'): TStringList; overload;
 procedure FindAllFiles(AList: TStrings; const SearchPath: String;
-  SearchMask: String = ''; SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
+  const SearchMask: String = ''; SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
   MaskSeparator: char = ';'; PathSeparator: char = ';'); overload;
 
 function FindAllDirectories(const SearchPath: string;
find2.diff (3,798 bytes)   

Serge Anvarov

2020-09-21 02:28

reporter   ~0125693

I think in the TFileSearcher.Search function variables p1 and p2 must be of type SizeInt, not Integer.
See https://www.freepascal.org/docs-html/rtl/system/length.html and https://www.freepascal.org/docs-html/rtl/system/copy.html and https://www.freepascal.org/docs-html/rtl/strutils/posex.html

Bart Broersma

2020-09-21 12:10

developer   ~0125702

You change the default behaviour of FindAllFiles to be case-sensitive on *nix (or did I misunderstand your patch)?
If so, that is not acceptable.
At least it should be configurable in the call to FindAllFiles and the default behavious should remains as it is now.

Also: please separate the functional changes (case-sensitive vs case-insensitive) from te optimization (const paramters).

CudaText man_

2020-09-21 12:16

reporter   ~0125703

So, new1.diff

- p1,p2: SizeInt
- removed changed behaviour
new1.diff (3,309 bytes)   
Index: components/lazutils/fileutil.inc
===================================================================
--- components/lazutils/fileutil.inc	(revision 63900)
+++ components/lazutils/fileutil.inc	(working copy)
@@ -656,7 +656,7 @@
 end;
 
 procedure FindAllFiles(AList: TStrings; const SearchPath: String;
-  SearchMask: String; SearchSubDirs: Boolean; DirAttr: Word;
+  const SearchMask: String; SearchSubDirs: Boolean; DirAttr: Word;
   MaskSeparator: char; PathSeparator: char);
 var
   Searcher: TListFileSearcher;
@@ -672,7 +672,7 @@
   end;
 end;
 
-function FindAllFiles(const SearchPath: String; SearchMask: String;
+function FindAllFiles(const SearchPath: String; const SearchMask: String;
   SearchSubDirs: Boolean; DirAttr: Word;
   MaskSeparator: char; PathSeparator: char): TStringList;
 begin
@@ -765,7 +765,7 @@
   FSearching := False;
 end;
 
-procedure TFileSearcher.Search(ASearchPath: String; ASearchMask: String;
+procedure TFileSearcher.Search(const ASearchPath: String; const ASearchMask: String;
   ASearchSubDirs: Boolean; CaseSensitive: Boolean = False);
 var
   MaskList: TMaskList;
@@ -836,9 +836,9 @@
   end;
 
 var
-  p: SizeInt;
+  p1, p2: SizeInt;
+  i: Integer;
   Dir: String;
-  i: Integer;
   OtherDir: String;
   MaskOptions: TMaskOptions;
 begin
@@ -855,12 +855,14 @@
   FSearching := True;
   SearchDirectories:=TStringList.Create;
   try
-    while ASearchPath<>'' do begin
-      p:=Pos(FPathSeparator,ASearchPath);
-      if p<1 then
-        p:=length(ASearchPath)+1;
-      Dir:=ResolveDots(LeftStr(ASearchPath,p-1));
-      Delete(ASearchPath,1,p);
+    p1:=1;
+    while p1<=Length(ASearchPath) do
+    begin
+      p2:=PosEx(FPathSeparator,ASearchPath,p1);
+      if p2<1 then
+        p2:=length(ASearchPath)+1;
+      Dir:=ResolveDots(Copy(ASearchPath,p1,p2-p1));
+      p1:=p2+1;
       if Dir='' then continue;
       Dir:=ChompPathDelim(Dir);
       for i:=SearchDirectories.Count-1 downto 0 do
Index: components/lazutils/fileutil.pas
===================================================================
--- components/lazutils/fileutil.pas	(revision 63900)
+++ components/lazutils/fileutil.pas	(working copy)
@@ -145,7 +145,7 @@
     procedure DoFileFound; virtual;
   public
     constructor Create;
-    procedure Search(ASearchPath: String; ASearchMask: String = '';
+    procedure Search(const ASearchPath: String; const ASearchMask: String = '';
       ASearchSubDirs: Boolean = True; CaseSensitive: Boolean = False);
   public
     property MaskSeparator: char read FMaskSeparator write FMaskSeparator;
@@ -180,11 +180,11 @@
     constructor Create(AList: TStrings);
   end;
 
-function FindAllFiles(const SearchPath: String; SearchMask: String = '';
+function FindAllFiles(const SearchPath: String; const SearchMask: String = '';
   SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
   MaskSeparator: char = ';'; PathSeparator: char = ';'): TStringList; overload;
 procedure FindAllFiles(AList: TStrings; const SearchPath: String;
-  SearchMask: String = ''; SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
+  const SearchMask: String = ''; SearchSubDirs: Boolean = True; DirAttr: Word = faDirectory;
   MaskSeparator: char = ';'; PathSeparator: char = ';'); overload;
 
 function FindAllDirectories(const SearchPath: string;
new1.diff (3,309 bytes)   

Bart Broersma

2020-10-10 11:50

developer   ~0126209

Last edited: 2020-10-10 11:50

View 3 revisions

Applied last patch.
Once we ditch fpc 3.0.4 (when 3.2.2 is released), the StrUtils isn't needed anymore, because Pos() supports the index parameter then.
We could ifdef this now, or wait until fpc 3.2.2.
It would be nice if you would supply a new patch then (or now with the ifdef's for the compiler version).

Issue History

Date Modified Username Field Change
2020-09-21 01:02 CudaText man_ New Issue
2020-09-21 01:02 CudaText man_ File Added: find.diff
2020-09-21 01:33 CudaText man_ Note Added: 0125692
2020-09-21 01:33 CudaText man_ File Added: find2.diff
2020-09-21 02:28 Serge Anvarov Note Added: 0125693
2020-09-21 12:10 Bart Broersma Note Added: 0125702
2020-09-21 12:10 Bart Broersma Assigned To => Bart Broersma
2020-09-21 12:10 Bart Broersma Status new => feedback
2020-09-21 12:10 Bart Broersma LazTarget => -
2020-09-21 12:16 CudaText man_ Note Added: 0125703
2020-09-21 12:16 CudaText man_ File Added: new1.diff
2020-09-21 12:16 CudaText man_ Status feedback => assigned
2020-10-10 11:50 Bart Broersma Status assigned => resolved
2020-10-10 11:50 Bart Broersma Resolution open => fixed
2020-10-10 11:50 Bart Broersma Note Added: 0126209
2020-10-10 11:50 Bart Broersma Note Edited: 0126209 View Revisions
2020-10-10 11:50 Bart Broersma Note Edited: 0126209 View Revisions
2020-10-10 12:02 Bart Broersma Fixed in Version => 2.2
2020-10-10 12:02 Bart Broersma Target Version => 2.2
2020-10-10 12:02 Bart Broersma Fixed in Revision => r63989
2020-10-10 12:02 Bart Broersma LazTarget - => 2.2
2020-10-11 15:03 CudaText man_ Status resolved => closed