View Issue Details

IDProjectCategoryView StatusLast Update
0038214FPCRTLpublic2021-02-02 20:22
ReporterReinhard Kalinke Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Summary0038214: Inconsistency between TStringList SetValueFromIndex and SetValue
DescriptionSetValueFromIndex deletes an entry when Value is an empty string, SetValue doesn't. While the former is Delphi compatible the latter isn't.
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Activities

Reinhard Kalinke

2020-12-13 23:22

reporter   ~0127599

For a discussion and suggestion also see:

https://forum.lazarus.freepascal.org/index.php/topic,52497.0.html

Bart Broersma

2021-01-02 18:31

reporter   ~0128029

Attached sample project "sl.lpr".
It outputs:

FPC
Count=3
0: "Key0=Value0"
1: "Key1="
2: "NonExistingKey="

Delphi
Count=1
0: "Key0=Value0"

Possible patch "stringlist.setvalue.diff" attached.
After applying the output for FPC is the same as in Delphi 7
stringlist.setvalue.diff (612 bytes)   
Index: rtl/objpas/classes/stringl.inc
===================================================================
--- rtl/objpas/classes/stringl.inc	(revision 47193)
+++ rtl/objpas/classes/stringl.inc	(working copy)
@@ -700,9 +700,17 @@
   CheckSpecialChars;
   L:=IndexOfName(Name);
   if L=-1 then
-   Add (Name+FNameValueSeparator+Value)
+    begin
+    if Value<>'' then
+      Add (Name+FNameValueSeparator+Value)
+    end
   else
-   Strings[L]:=Name+FNameValueSeparator+value;
+    begin
+    if Value='' then
+      Delete(L)
+    else
+      Strings[L]:=Name+FNameValueSeparator+value;
+    end;
 end;
 
 
stringlist.setvalue.diff (612 bytes)   
sl.lpr (618 bytes)   
program sl;

{$apptype console}
{$ifdef fpc}
{$mode objfpc}
{$h+}
{$endif}

uses
  classes;

var
  L: TstringList;
  i: Integer;

begin
  {$ifdef fpc}
  writeln('FPC');
  {$else}
  writeln('Delphi');
  {$endif}
  L := TStringList.Create;
  L.Add('Key0=Value0');
  L.Add('Key1=Value1');
  L.Values['Key1'] := '';
  L.Values['NonExistingKey'] := '';
  writeln('Count=',L.Count);
  for i := 0 to L.Count-1 do
    writeln(i,': "',L[i],'"');
  L.Free;
  {
  FPC
  Count=3
  0: "Key0=Value0"
  1: "Key1="
  2: "NonExistingKey="

  Delphi
  Count=1
  0: "Key0=Value0"

  }

end.

sl.lpr (618 bytes)   

Reinhard Kalinke

2021-02-01 18:13

reporter   ~0128726

I'm still pleading for *not* going for Delphi compatibility here for this is quite clearly a bug in Delphi. And I suppose that there is rather code out there that works around this than code that really relies on it.

cf https://forum.lazarus.freepascal.org/index.php/topic,52497.msg387552.html#msg387552 and above

Bart Broersma

2021-02-01 20:49

reporter   ~0128729

The inconsistency between SetValueFromIndex and SetValue is a bug in itself.
TStringList is a rather basic component and as such should be as much Delphi compatible as possible.
You could plead for the old behaviour to be configurable by means of an option or property...

Reinhard Kalinke

2021-02-01 21:23

reporter   ~0128730

"The inconsistency between SetValueFromIndex and SetValue is a bug in itself."

No objections, it clearly is. But this can be fixed in two ways, nevertheless.

"TStringList is a rather basic component and as such should be as much Delphi compatible as possible."

Even if the behaviour that is or would be compatible is complete nonsense? Where's the point in allowing an empty key, let alone adding one?

"You could plead for the old behaviour to be configurable by means of an option or property... "

I'm not sure what you mean by 'old behaviour' here, but this could indeed be an option - either 'Delphi compatible'-mode or code that makes sense, ie allowing empty values but not keys.

Juha Manninen

2021-02-02 17:07

reporter   ~0128742

One can do:
 L.Add('=Value1');
Or anything.
I am pleading you guys to deprecate the Values[] syntax and all functionality related to it.
A proper string to string map container should be used then. This feature of TStringList was a hack made by somebody in a hurry in early Borland days. When was it implemented? I don't know.
It is used in Lazarus and FPC libraries unfortunately. The slowdown is maybe 100-fold compared to a proper container. Uhhh...

Bart Broersma

2021-02-02 20:22

reporter   ~0128744

TStringList is a factotum by design ;-)
For simple use cases this is not a problem.
While Juha's argument in itself is probably correct, it divertes the discussion from the bug in question.

It's up to the fpc devels to decide what to do with this.
I have no software that relies on either way it works (now or as proposed by my patch).
I just took it upon me to actually write a patch, since I figured I should be capable enough doing so.

Issue History

Date Modified Username Field Change
2020-12-13 17:04 Reinhard Kalinke New Issue
2020-12-13 23:22 Reinhard Kalinke Note Added: 0127599
2021-01-02 18:31 Bart Broersma Note Added: 0128029
2021-01-02 18:31 Bart Broersma File Added: stringlist.setvalue.diff
2021-01-02 18:31 Bart Broersma File Added: sl.lpr
2021-02-01 18:13 Reinhard Kalinke Note Added: 0128726
2021-02-01 20:49 Bart Broersma Note Added: 0128729
2021-02-01 21:23 Reinhard Kalinke Note Added: 0128730
2021-02-02 17:07 Juha Manninen Note Added: 0128742
2021-02-02 20:22 Bart Broersma Note Added: 0128744