LazUtils: consider to add 'const S' for these string funcs

[AlexTP]

Found these by RegEx (regex is shown in the 1st line of report).

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

1. +Search "\(\w+\s*:\s*\w*string\b". Report with [styles].
2.         <avglvltree.pas>: #1
3.                 <1023>:   procedure E(Msg: string);
4.         <compwriterpas.pas>: #4
5.                 < 133>:     procedure WriteCollection(PropName: string; Collection: TCollection); virtual;
6.                 < 134>:     function ShortenFloat(s: string): string; virtual;
7.                 <1139>: procedure TCompWriterPas.WriteCollection(PropName: string;
8.                 <1383>: function TCompWriterPas.ShortenFloat(s: string): string;
9.         <fileutil.inc>: #1
10.                 < 544>:   procedure Add(NewFilename: string);
11.         <laz2_xmlcfg.pas>: #8
12.                 < 162>:     procedure WriteProperty(Path: String; Instance: TObject;
13.                 < 165>:     procedure ReadProperty(Path: String; Instance: TObject;
14.                 < 169>:     procedure WriteObject(Path: String; Obj: TObject;
15.                 < 171>:     procedure ReadObject(Path: String; Obj: TObject;
16.                 <1115>: procedure TRttiXMLConfig.WriteObject(Path: String; Obj: TObject;
17.                 <1133>: procedure TRttiXMLConfig.WriteProperty(Path: String; Instance: TObject;
18.                 <1275>: procedure TRttiXMLConfig.ReadProperty(Path: String; Instance: TObject;
19.                 <1416>: procedure TRttiXMLConfig.ReadObject(Path: String; Obj: TObject;
20.         <laz2_xmlutils.pas>: #2
21.                 < 109>:     function Locate(uri: PXMLUtilString; localName: PXMLUtilChar; localLength: Integer): Boolean;
22.                 < 898>: function TDblHashArray.Locate(uri: PXMLUtilString; localName: PXMLUtilChar; localLength: Integer): Boolean;
23.         <lazconfigstorage.pas>: #8
24.                 <  45>:     procedure WriteProperty(Path: String; Instance: TPersistent;
25.                 <  48>:     procedure ReadProperty(Path: String; Instance: TPersistent;
26.                 <  82>:     procedure WriteObject(Path: String; Obj: TPersistent;
27.                 <  84>:     procedure ReadObject(Path: String; Obj: TPersistent;
28.                 < 364>: procedure TConfigStorage.WriteProperty(Path: String; Instance: TPersistent; PropInfo: Pointer;
29.                 < 485>: procedure TConfigStorage.ReadProperty(Path: String; Instance: TPersistent; PropInfo: Pointer;
30.                 < 776>: procedure TConfigStorage.WriteObject(Path: String; Obj: TPersistent; DefObject: TPersistent;
31.                 < 795>: procedure TConfigStorage.ReadObject(Path: String; Obj: TPersistent; DefObject: TPersistent;
32.         <lazfilecache.pas>: #2
33.                 < 120>:   TOnFileExistsCached = function(Filename: string): boolean of object;
34.                 < 121>:   TOnFileAgeCached = function(Filename: string): longint of object;
35.         <lazfileutils.inc>: #1
36.                 < 252>:   function SplitDirs(Dir: String; out Dirs: TDirArr): Integer;
37.         <lazfileutils.pas>: #10
38.                 <  42>: function DirPathExists(DirectoryName: string): boolean;
39.                 <  49>: function ForceDirectory(DirectoryName: string): boolean;
40.                 < 164>: function ExtractFileRoot(FileName: String): String;
41.                 < 168>: function GetDarwinSystemFilename(Filename: string): string;
42.                 < 169>: function GetDarwinNormalizedFilename(Filename: string; nForm:Integer=2): string;
43.                 < 488>: function GetDarwinSystemFilename(Filename: string): string;
44.                 < 542>: function GetDarwinNormalizedFilename(Filename: string; nForm:Integer=2): string;
45.                 < 594>: function DirPathExists(DirectoryName: string): boolean;
46.                 < 619>: function ForceDirectory(DirectoryName: string): boolean;
47.                 <1584>: function ExtractFileRoot(FileName: String): String;
48.         <lazlogger.pas>: #12
49.                 <  52>:     procedure SetLogName(AValue: String);
50.                 < 139>:     procedure SetEnvironmentForLogFileName(AValue: String);
51.                 < 141>:     procedure SetParamForLogFileName(AValue: String);
52.                 < 149>:     procedure SetLogName(AValue: String);
53.                 < 165>:     procedure DoDbgOut(s: string; AGroup: PLazLoggerLogGroup = nil); override;
54.                 < 166>:     procedure DoDebugLn(s: string; AGroup: PLazLoggerLogGroup = nil); override;
55.                 < 445>: procedure TLazLoggerFileHandle.SetLogName(AValue: String);
56.                 < 559>: procedure TLazLoggerFile.SetEnvironmentForLogFileName(AValue: String);
57.                 < 575>: procedure TLazLoggerFile.SetParamForLogFileName(AValue: String);
58.                 < 607>: procedure TLazLoggerFile.SetLogName(AValue: String);
59.                 < 729>: procedure TLazLoggerFile.DoDbgOut(s: string; AGroup: PLazLoggerLogGroup);
60.                 < 781>: procedure TLazLoggerFile.DoDebugLn(s: string; AGroup: PLazLoggerLogGroup);
61.         <lazloggerbase.pas>: #8
62.                 < 192>:     procedure DebugLnEnter(s: string; const Args: array of const); overload;
63.                 < 203>:     procedure DebugLnExit(s: string; const Args: array of const); overload;
64.                 < 272>:     procedure SetParamForEnabledLogGroups(AValue: String);
65.                 < 763>: procedure TLazLogger.DoDbgOut(s: string; AGroup: PLazLoggerLogGroup);
66.                 < 768>: procedure TLazLogger.DoDebugLn(s: string; AGroup: PLazLoggerLogGroup);
67.                 < 977>: procedure TLazLogger.DebugLnEnter(s: string; const Args: array of const);
68.                 <1014>: procedure TLazLogger.DebugLnExit(s: string; const Args: array of const);
69.                 <1185>: procedure TLazLoggerWithGroupParam.SetParamForEnabledLogGroups(AValue: String);
70.         <lazloggerdummy.pas>: #2
71.                 < 396>: procedure TLazLogger.DebugLnEnter(s: string; const Args: array of const);
72.                 < 420>: procedure TLazLogger.DebugLnExit(s: string; const Args: array of const);
73.         <LazLoggerImpl.inc>: #2
74.                 <  98>: procedure DebugLnEnter(s: string; const Args: array of const);
75.                 < 137>: procedure DebugLnExit(s: string; const Args: array of const);
76.         <LazLoggerIntf.inc>: #2
77.                 <  26>: procedure DebugLnEnter(s: string; const Args: array of const); {inline;} overload;
78.                 <  37>: procedure DebugLnExit(s: string; const Args: array of const); {inline;} overload;
79.         <lazloggerprofiling.pas>: #16
80.                 <  64>: function DbgsMemUsed(AFormat: String = '%0:d'): string;
81.                 <  65>: function DbgsTimeUsed(AFormat: String = '%0:n'): string;
82.                 <  67>: procedure DbgStartTimer(AName: String);
83.                 <  68>: procedure DbgStopTimer(AName: String);
84.                 <  69>: procedure DbgStartMemWatch(AName: String);
85.                 <  70>: procedure DbgStopMemWatch(AName: String);
86.                 <  72>: function DbgsMemUsed(AFormat: String; AName: String): string;
87.                 <  73>: function DbgsTimeUsed(AFormat: String; AName: String): string;
88.                 < 110>: function DbgsMemUsed(AFormat: String): string;
89.                 < 126>: function DbgsTimeUsed(AFormat: String): string;
90.                 < 142>: procedure DbgStartTimer(AName: String);
91.                 < 161>: procedure DbgstopTimer(AName: String);
92.                 < 178>: procedure DbgStartMemWatch(AName: String);
93.                 < 197>: procedure DbgStopMemWatch(AName: String);
94.                 < 209>: function DbgsMemUsed(AFormat: String; AName: String): string;
95.                 < 225>: function DbgsTimeUsed(AFormat: String; AName: String): string;
96.         <lazstringutils.pas>: #2
97.                 <  45>: function IsNumeric(s: String): Boolean;
98.                 < 204>: function IsNumeric(s: String): Boolean;
99.         <lazunicode.pas>: #5
100.                 <  53>:   function StringOfCodePoint(ACodePoint: String; N: Integer): String;
101.                 <  97>:   //operator Enumerator(A: String): TCodePointEnumerator;
102.                 < 102>:   operator Enumerator(A: String): TUnicodeCharacterEnumerator;
103.                 < 235>: function StringOfCodePoint(ACodePoint: String; N: Integer): String;
104.                 < 315>: operator Enumerator(A: String): TUnicodeCharacterEnumerator;
105.         <lazutf16.pas>: #2
106.                 <  47>: function IsUTF16StringValid(AStr: UnicodeString): Boolean;
107.                 < 245>: function IsUTF16StringValid(AStr: UnicodeString): Boolean;
108.         <lazutf8.pas>: #16
109.                 < 138>: function UTF8ProperCase(AInStr: string; const WordDelims: TSysCharSet): string;
110.                 < 141>: function UTF8StringOfChar(AUtf8Char: String; N: Integer): String;
111.                 < 142>: function UTF8AddChar(AUtf8Char: String; const S: String; N: Integer): String;
112.                 < 143>: function UTF8AddCharR(AUtf8Char: String; const S: String; N: Integer): String;
113.                 < 159>: function UTF8WrapText(S: string; MaxCol: integer): string; overload;
114.                 < 161>: function IsAscii(S: string): Boolean; // String has only ASCII characters.
115.                 < 162>: function IsPureAscii(S: string): Boolean; inline;
116.                 < 180>: function Utf8EscapeControlChars(S: String; EscapeMode: TEscapeMode = emPascal; CustomStrings: PEscapedStrings = nil; AMaxGrowFactor: Cardinal = 0): String;
117.                 <1317>: function UTF8ProperCase(AInStr: string; const WordDelims: TSysCharSet): string;
118.                 <3032>: function Utf8EscapeControlChars(S: String; EscapeMode: TEscapeMode; CustomStrings: PEscapedStrings; AMaxGrowFactor: Cardinal): String;
119.                 <3130>: function UTF8StringOfChar(AUtf8Char: String; N: Integer): String;
120.                 <3176>: function UTF8AddChar(AUtf8Char: String; const S: String; N: Integer): String;
121.                 <3187>: function UTF8AddCharR(AUtf8Char: String; const S: String; N: Integer): String;
122.                 <3379>: function UTF8WrapText(S: string; MaxCol: integer): string;
123.                 <3384>: function IsAscii(S: string): Boolean;
124.                 <3394>: function IsPureAscii(S: string): Boolean;
125.         <lazutilities.pas>: #3
126.                 <  19>:   TGetSkipCheckByKey = function(AKey: String): Boolean;
127.                 <  22>: function GetSkipCheckByKey(AKey: String): Boolean;
128.                 <  62>: function GetSkipCheckByKey(AKey: String): Boolean;
129.         <lcsvutils.pas>: #2
130.                 <  25>:   procedure LoadFromCSVFile(aFilename: string; AProc: TCSVRecordProc;
131.                 < 278>: procedure LoadFromCSVFile(aFilename: string; AProc: TCSVRecordProc;
132.         <masks.pas>: #7
133.                 < 235>:     procedure SetMask(AValue: String); virtual;
134.                 < 261>:     procedure SetMask(AValue: String); override;
135.                 < 308>:     procedure SetMask(AValue: String); virtual;
136.                 < 713>: procedure TMaskUTF8.SetMask(AValue: String);
137.                 <1255>: procedure TWindowsMaskUTF8.SetMask(AValue: String);
138.                 <1335>:   function OptionalQMarksAtEnd(aMask: String): String;
139.                 <1524>: procedure TMaskList.SetMask(AValue: String);
140.         <translations.pas>: #3
141.                 < 191>: function GetLanguageIDFromLocaleName(LocaleName: string): TLanguageID;
142.                 < 357>: function ExtractFormatArgs(S: String; out ArgumentError: Integer): String;
143.                 < 465>: function GetLanguageIDFromLocaleName(LocaleName: string): TLanguageID;
144.         <utf8process.pp>: #2
145.                 <  36>: function FindFilenameOfCmd(ProgramFilename: string): string;
146.                 < 115>: function FindFilenameOfCmd(ProgramFilename: string): string;
147.         <winlazfileutils.inc>: #2
148.                 <  19>:   function TryGetSymlinkTarget(Fn: UnicodeString; out SymlinkRec: TUnicodeSymlinkRec): Boolean;
149.                 <  33>:   function TryGetSymlinkTarget(Fn: UnicodeString; out SymlinkRec: TUnicodeSymlinkRec): Boolean; inline;
150.         <examples\LookupStringList\main.pas>: #2
151.                 <  31>:     procedure UpdateDuplicates(aDuplicateCount: string);
152.                 <  46>: procedure TForm1.UpdateDuplicates(aDuplicateCount: string);
153.  

[Martin_fr]

There may be reasons why some of them MUST NOT have "const".

I haven't looked through those listed, but we had crashes because of using const.

One reason where it must not be used is if the code may execute events (callbacks), including by nested calls inside the code.

Consider below example (that I have given in similar form before)

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

1. var MyVal: string; // may be field in a string;
2.  
3. procedure Foo(const s: string);
4. begin
5.   // will at some point call MyEvent
6. end;
7.  
8. procedure TheEvent;
9. begin
10.   MyVal := NewVal;
11. end;
12.  
13. procedure Start;
14. begin
15.   Foo(MyVal);
16. end;

This can crash. It can behave strange. It can sometimes (even often) do what is expected, making it even harder to find the bug, when it suddenly goes wrong.

[Martin_fr]

Also your regex doesn't even check if the string param is being assigned within the procedure, in which case it would break compilation...

[marcov]

And changing virtual methods might break derivative code, so at least save that for major versions.

But problem cases are rare and dodgy.

[AlexTP]

Of course you are right. I gave the wide list of 'possible places' which I want to change, callbacks / virtual_methods are special cases of course (must not be changed).

[Martin_fr]

Of course you are right. I gave the wide list of 'possible places' which I want to change, callbacks / virtual_methods are special cases of course (must not be changed).

It is not just "callbacks" that must not be changed. But all and any code that invokes a callback. Even if that callback does not have the string in its parameters.

And on a case by case base,  code that (anywhere within any nested execution) modifies any non-local string variable may also break if changed.

And even if we can prove a function save today, if it has deep nested calls, then we need to be sure that non of them will ever change to make it unsafe.


We have in the past spent good amount of times tracing bugs from incorrectly used "const" param. The compiler will not give you any warnings on it (and that is documented / and explained several times on this forum).

[JuhaManninen]

The whole obsession for const parameters is ridiculous. The speed benefit is marginal but it can cause nasty bugs which are hard to find .
AlexTP, please try to get rid of that obsession.

[AlexTP]

Ok, to avoid hard-to-find bugs, keep it as is, sorry.

[Martin_fr]

To elaborate

function TCompWriterPas.ShortenFloat(s: string): string;

That looks safe, so it could have the "const".
But the code modifies "s", therefore it can't. So it would be necessary to rewrite the code, and then you have to check if the rewrite does not introduce any new refcount changes (which would undo any gains that the "const" would bring).

Might actually be possible in that particular case.

That then leaves the last question: Is that code called in a way that needs every last bit of speed? Is TCompWriterPas used in code that performs lots of computations, and calls it millions of times in a loop?
Or are we introducing a change for the sake of changing?

That is, if there was work on that code, and if const is safe, and if the rewrite to make const working would keep readability and not introduce other ref-counts, then I wouldn't mind adding it.

But, if I (or probably anyone in the team) will have to check the correctness of a patch, to gain nothing measurable (as the function is not called in a manner that the savings would make a difference)... Then there is no interest (at least not on my side).


So, if you can identify code
- that is currently save (when it has the const param)
- can be kept future save (no risk of problem introduction in nested calls of any code called by the proc)
- the function has use cases that would allow some real gains
- const works without need of change in the code (any code in or outside the funct),
- or the changes are minor, keep readability, and are appropriate for the gains.

Then we can look at it.


Right now, we still have legacy code that is unsafe due to const... But due to "virtual" or other constraints not easy to change...

[Martin_fr]

I do for example use const param in some of synedits code, because it may process text with millions of lines, and make lots of calls. There may be legacy on functions that aren't called that often (and would not pass all the checks / had to learn the lessons myself too).

Or in fpdebug, if collecting history snapshots with auto continue breakpoints, that could be hit thousands of times then getting watches and stacks as fast as possible is important...

[marcov]

The whole obsession for const parameters is ridiculous. The speed benefit is marginal but it can cause nasty bugs which are hard to find .
AlexTP, please try to get rid of that obsession.

It may be marginal for GUI purposes, but for string processing code avoiding unnecessary memory allocations is certainly noticeable.

The question is where you draw the border. Some GUI events  can go very fast because they only update the backing storage, rather than force a repaint.

[440bx]

IMO, "const" should _not_ be used as an optimization but as a documentation feature and to inform the compiler that assignments to the const identifier should not be allowed.

when used with structures, "constref" should be preferred because it makes it explicit (instead of implicit) that the parameter is being passed by reference (const may or may not pass by reference, it depends on the parameter size and when the parameter is a pointer (as longstrings are), that can be misleading.)

[Thaddy]

The whole obsession for const parameters is ridiculous. The speed benefit is marginal but it can cause nasty bugs which are hard to find .
AlexTP, please try to get rid of that obsession.

Why? He is asking for something that is a matter of correctness?
I am also not aware it can cause any nasty bugs. Example appreciated.
I am aware that in heavy string handling situations it simply avoids a lot of common problems with managed string types.
The compiler can better optimize when const is used correctly for managed types. Simple as that.
It can drop implicit try finally and refcount.

[Martin_fr]

It may be marginal for GUI purposes, but for string processing code avoiding unnecessary memory allocations is certainly noticeable.

"procedure foo(const s: ansistring);"

The "const" does not change any memory allocations.
That is compared to the same code just without the "const" declaration (which means the code would still not modify the string and not copy it).

All it does is, it skips an InterlockedIncrement/Decrement. (Which as a locked mem access has a tiny bit of cost).

And yes, when you have GUI code, then the GUI stuff usually takes a lot more time.


There are other things to speed up work with strings.

First, don't use heaptrc. If strings get modified a lot, you get a lot of mem re-alloc. And heaptrc really slows that. (I have some bits of code that do heavy string processing, and with heaptrc they slow down by more than factor 10).

Second, use UniqueString and pchar access if you need to modify many "char"s inside a string. That avoids a repeated ref-count check on every write access.

[Martin_fr]

IMO, "const" should _not_ be used as an optimization but as a documentation feature and to inform the compiler that assignments to the const identifier should not be allowed.

when used with structures, "constref" should be preferred because it makes it explicit (instead of implicit) that the parameter is being passed by reference (const may or may not pass by reference, it depends on the parameter size and when the parameter is a pointer (as longstrings are), that can be misleading.)

That is 2 different features. Even though they have huge overlap.

One is code quality, the other is optimization. Both features have their place, and have good use cases. Sadly we only have one of them implemented.
And for structures, if it is about optimization, then you would not care if it is by ref, so const will be better, as the compiler can generate whatever performs better.