Recent

Author Topic: Publuc property of type TStringList  (Read 713 times)

zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Publuc property of type TStringList
« on: November 20, 2020, 07:23:26 am »
Hi guys.

I am trying to use a public property of type TStringList. Are memory leaks possible in this code like this?

Code: Pascal  [Select][+][-]
  1. type
  2.   { TForm1 }
  3.   TForm1 = class(TForm)
  4. ...
  5.     procedure FormClose(Sender: TObject; var CloseAction: TCloseAction);
  6.     procedure FormCreate(Sender: TObject);
  7.   private
  8.     FMySL: TStringList;
  9.     procedure SetMySL(AValue: TStringList);
  10.   public
  11.     property MySL: TStringList read FMySL write SetMySL;
  12. ...
  13.   end;
  14.  
  15. procedure TForm1.SetMySL(AValue: TStringList);
  16. begin
  17.   if FMySL=AValue then Exit;
  18.   if Assigned(FMySL) then FreeAndNil(FMySL);
  19.   FMySL:=AValue;
  20. end;
  21.  
  22. procedure TForm1.FormCreate(Sender: TObject);
  23. begin
  24.   MySL:= TStringList.Create;
  25. ...
  26. end;
  27.  
  28. procedure TForm1.FormClose(Sender: TObject; var CloseAction: TCloseAction);
  29. begin
  30.   if Assigned(MySL) then MySL.Free;
  31. end;
  32.  
  33. //using
  34. procedure TForm1.Button3Click(Sender: TObject);
  35. var
  36.   i: PtrInt;
  37. begin
  38.   with MySL do
  39.   begin
  40.     Clear;
  41.     for i:= 0 to 9 do
  42.     begin
  43.       Add('item_' + FormatDateTime('hh:nn:ss:zzz', Now));
  44.       Sleep(50);
  45.     end;
  46.   end;
  47.   Memo1.Lines.Assign(MySL);
  48. end;
  49.  
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

rvk

  • Hero Member
  • *****
  • Posts: 4452
Re: Publuc property of type TStringList
« Reply #1 on: November 20, 2020, 07:32:05 am »
Don't do MySL.Free in FormClose but in FormDestroy.

The opposite of FormCreate is FormDestroy.
You are guaranteed both are executed.
(FormClose is not guaranteed)


zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Re: Publuc property of type TStringList
« Reply #2 on: November 20, 2020, 07:38:09 am »
@rvk
Thank you for your comment.

Can the code inside procedure SetMySL be considered correct?
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

lucamar

  • Hero Member
  • *****
  • Posts: 3430
Re: Publuc property of type TStringList
« Reply #3 on: November 20, 2020, 07:41:27 am »
Yes, it looks correct see but I would do things like this:

Code: Pascal  [Select][+][-]
  1. type
  2.   { TForm1 }
  3.   TForm1 = class(TForm)
  4. ...
  5.     procedure FormDestroy(Sender: TObject);
  6.     procedure FormCreate(Sender: TObject);
  7.   private
  8.     FMySL: TStringList;
  9. ...
  10.   end;
  11.  
  12. procedure TForm1.FormCreate(Sender: TObject);
  13. begin
  14.   FMySL:= TStringList.Create;
  15. ...
  16. end;
  17.  
  18. procedure TForm1.FormDestroy(Sender: TObject);
  19. begin
  20.   if Assigned(FMySL) then FMySL.Free;
  21. end;
  22.  
  23. //using
  24. procedure TForm1.Button3Click(Sender: TObject);
  25. var
  26.   i: PtrInt;
  27. begin
  28.   with FMySL do
  29.   begin
  30.     Clear;
  31.     for i:= 0 to 9 do
  32.     begin
  33.       Add('item_' + FormatDateTime('hh:nn:ss:zzz', Now));
  34.       Sleep(50);
  35.     end;
  36.   end;
  37.   Memo1.Lines.Assign(FMySL);
  38. end;

I wouldn't bother with the property, just access the field by itself. Generally speaking a property, with its getter and setter, is nice to have when those involve more than simply assigning it like, say, checking some values to set it some way or other, or when it depends on other fields/properties, etc. For a simple list which is used by itself, declaring and using the field should be enough; no need to further complicate things.
« Last Edit: November 20, 2020, 07:44:29 am by lucamar »
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus/FPC 2.0.8/3.0.4 & 2.0.10/3.2.0 - 32/64 bits on:
(K|L|X)Ubuntu 12..18, Windows XP, 7, 10 and various DOSes.

zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Re: Publuc property of type TStringList
« Reply #4 on: November 20, 2020, 07:48:42 am »
no need to further complicate things.
Hi lucamar.

Thank U for answer.
I have declared this property for a reason. I want to access it from other units in order to pass the contents of the list between forms.
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

wp

  • Hero Member
  • *****
  • Posts: 7948
Re: Publuc property of type TStringList
« Reply #5 on: November 20, 2020, 09:49:18 am »
Code: Pascal  [Select][+][-]
  1. type
  2.   { TForm1 }
  3.   TForm1 = class(TForm)
  4.   private
  5.     FMySL: TStringList;
  6.     procedure SetMySL(AValue: TStringList);
  7.   public
  8.     property MySL: TStringList read FMySL write SetMySL;
  9. ...
  10.   end;
  11.  
  12. procedure TForm1.SetMySL(AValue: TStringList);
  13. begin
  14.   if FMySL=AValue then Exit;
  15.   if Assigned(FMySL) then FreeAndNil(FMySL);
  16.   FMySL:=AValue;
  17. end;
  18.  

This is a risky construction. It is the calling program which created the stringlist passed to SetMySL as a parameter, and therefore normally it the responsibility of the calling program to destroy the stringlist. But since you just copy the pointer to TForm1 the stringlist will be gone and your program will not notice it; it even won't help when the calling routine sets the destroyed stringlist to nil -- this will affect the local variable in the calling program, but not the copied value FMySL. A crash is very likely...

Better: Keep ownership of the stringlist at the calling routine and copy the stringlist items to FMySL, not the pointer to the stringlist. You could even go a step further and declare AValue as the more general type TStrings; sticking to TStringList would not allow you to use the Lines of a TMemo in SetMySL (because this is not a TStringList but a TTextStrings which inherits from TStrings without going the TStringList path of inheritance).

Code: Pascal  [Select][+][-]
  1. procedure TForm1.SetMySL(AValue: TStrings);
  2. begin
  3.   FMySL.Assign(AValue);  // This copies all properties of AValue over to FMySL
  4. end;
Mainly Lazarus trunk / fpc 3.2.0 / all 32-bit on Win-10, but many more...

lucamar

  • Hero Member
  • *****
  • Posts: 3430
Re: Publuc property of type TStringList
« Reply #6 on: November 20, 2020, 01:59:52 pm »
I have declared this property for a reason. I want to access it from other units in order to pass the contents of the list between forms.

If it's only that then declare it as a read-only property. That would allow using it, including modyfing its contents, wherever needed but keep the ownership (hence construction/destruction responsability)  in the container form object.
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus/FPC 2.0.8/3.0.4 & 2.0.10/3.2.0 - 32/64 bits on:
(K|L|X)Ubuntu 12..18, Windows XP, 7, 10 and various DOSes.

zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Re: Publuc property of type TStringList
« Reply #7 on: November 20, 2020, 09:33:53 pm »
If it's only that then declare it as a read-only property.

I can't declare this property just RO because I have to be able to change this property from another unit. For example:
Code: Pascal  [Select][+][-]
  1. procedure TForm1.Button2Click(Sender: TObject);
  2. var
  3.   i: Integer;
  4. begin
  5.   FrmMain.MySL.Clear;//here MySL this is public property of FrmMain
  6.   for i:= 0 to Pred(CheckListBox1.Items.Count) do
  7.     if not CheckListBox1.Checked[i] then FrmMain.MySL.Add(CheckListBox1.Items.Strings[i]);//copying unchecked items from chebklistbox to FrmMain.MySL
  8.  
  9.   Memo1.Lines.Assign(FrmMain.MySL);//checking copied lines
  10. end;

Better: Keep ownership of the stringlist at the calling routine and copy the stringlist items to FMySL, not the pointer to the stringlist.
Code: Pascal  [Select][+][-]
  1. procedure TForm1.SetMySL(AValue: TStrings);
  2. begin
  3.   FMySL.Assign(AValue);  // This copies all properties of AValue over to FMySL
  4. end;

Hi Werner.
Thank U for the answer. Sorry but your code raises an error
Code: Pascal  [Select][+][-]
  1. Project project1 raised exception class 'External: SIGSEGV'.
  2.  
  3.  In file 'unit1.pas' at line 90:
  4. FMySL.Assign(AValue);

In addition, using type TStrings instead of TStringList also causes errors on methods Clear, Add  etc.

Code: Pascal  [Select][+][-]
  1. Project project1 raised exception class 'RunError(211)' with message:
  2. Call to abstract method
  3.  
  4.  In file 'unit1.pas' at line 65:
  5. Clear;

I have attached a project archive for experiments
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

Sieben

  • Full Member
  • ***
  • Posts: 196
Re: Publuc property of type TStringList
« Reply #8 on: November 20, 2020, 09:43:39 pm »
As lucamar wrote, a readonly property would not prevent you from modifying it's contents like you do in line 7 of your first code snippet. Using Add, Clear, Assign etc is perfectly possible with a readonly property. Readonly just means you are not allowed to change the (pointer of the) object itself, but of course you can use and modify the contents of the object.
Lazarus 2.0.10, FPC 3.2.0, .deb install on Ubuntu Xenial 32 / Gtk2 / Unity7

Sieben

  • Full Member
  • ***
  • Posts: 196
Re: Publuc property of type TStringList
« Reply #9 on: November 20, 2020, 09:56:53 pm »
Had a look at your project. Don't use:

Code: Pascal  [Select][+][-]
  1. TStrings.Create;  

but always

Code: Pascal  [Select][+][-]
  1. TStringList.Create;  

because TStrings is in fact just an abstract class that should never be used in 'real' code. Suggesting a procedure with parameter type TStrings like wp just means that you can pass any descendant of TStrings, which makes the procedure more widely usable (or 'generic'). 

Edit: attached corrected sample project.
« Last Edit: November 20, 2020, 10:26:49 pm by Sieben »
Lazarus 2.0.10, FPC 3.2.0, .deb install on Ubuntu Xenial 32 / Gtk2 / Unity7

wp

  • Hero Member
  • *****
  • Posts: 7948
Re: Publuc property of type TStringList
« Reply #10 on: November 20, 2020, 10:20:14 pm »
Sorry but your code raises an error
As Sieben already wrote, create FMySL as a TStringList.

However, using my setter function where the items of the string passed as a parameter are only assigned to the internal list (FMySL), you cannot do "MySL := TStringList.Create" -- this code creates a stringlist and copies its items (there are none in the beginning!) to FMySL which does not yet exist --> crash. Rather than using "MySL" use the internal variable "FMySL":
Code: Pascal  [Select][+][-]
  1. procedure TForm1.FormCreate(Sender: TObject);
  2. begin
  3.   FMySL:= TStringList.Create;   // FMySL instead of MySL !!!
  4.   FillCheckListBox(Sender);
  5. end;
Mainly Lazarus trunk / fpc 3.2.0 / all 32-bit on Win-10, but many more...

Sieben

  • Full Member
  • ***
  • Posts: 196
Re: Publuc property of type TStringList
« Reply #11 on: November 20, 2020, 10:25:03 pm »
Correct of course, will change that in my original post to avoid confusion.
Lazarus 2.0.10, FPC 3.2.0, .deb install on Ubuntu Xenial 32 / Gtk2 / Unity7

zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Re: Publuc property of type TStringList
« Reply #12 on: November 21, 2020, 06:02:10 am »
Hi guys.

Thanks for your help and advice. I will try to apply the corrected code in a complex project.
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Re: Publuc property of type TStringList
« Reply #13 on: November 25, 2020, 07:11:50 am »
Hi guys.

Here is a schematic example of my real application. In my opinion, I was able to achieve the desired result using only TStringList instead of TStrings.

Can someone tell me if I wrote the setter correctly? Can TStrings be used instead of TStringList?

Code: Pascal  [Select][+][-]
  1. { TfrmMain }
  2.  
  3. //create and call detail form
  4. procedure TfrmMain.Button1Click(Sender: TObject);
  5. var
  6.   tmpForm: TfrmDetail;
  7. begin
  8.   FMyStr:= TStringList.Create;
  9.   try
  10.     FMyStr.Assign(ComboBox1.Items);
  11.     tmpForm:= TfrmDetail.Create(Self);
  12.     try
  13.       tmpForm.ShowModal;
  14.       FillList(ComboBox1, MyStr);
  15.       if ComboBox1.Items.Count > 0 then ComboBox1.ItemIndex:= 0;
  16.     finally
  17.       FreeAndNil(tmpForm);
  18.     end;
  19.   finally
  20.     FreeAndNil(FMyStr);
  21.   end;
  22. end;
  23.  
  24. //setter
  25. procedure TfrmMain.SetMyStr(AValue: TStringList);
  26. begin
  27.   if FMyStr=AValue then Exit;
  28.   //FMyStr:=AValue;
  29.   if Assigned(FMyStr) then FMyStr.Assign(AValue);
  30. end;
  31.  
  32. { TfrmDetail }
  33.  
  34. procedure TfrmDetail.Button1Click(Sender: TObject);
  35. var
  36.   i: PtrInt;
  37. begin
  38.   frmMain.MyStr.Clear;
  39.   for i:= 0 to Pred(CheckListBox1.Count) do
  40.     if CheckListBox1.Checked[i] then frmMain.MyStr.Add(CheckListBox1.Items.Strings[i]);
  41.  
  42.   Close;
  43. end;
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

zoltanleo

  • Sr. Member
  • ****
  • Posts: 289
Re: Publuc property of type TStringList
« Reply #14 on: November 25, 2020, 07:12:25 am »
screen
Win10 LTSC x64/Deb 10 amd64/Darwin Cocoa:
Lazarus x32/x64 2.1(r.64226); FPC 3.3.1 (r.47808), FireBird 3.0.7

Sorry for my bad English, I'm using translator ;)

 

TinyPortal © 2005-2018