CODEWITHSUNDEEP
delphimemory-leaksdelphi-7
SCAJason
SCAJason

Reputation: 29

Memory Leak in Complete Delphi Code using a tlist

Attached is complete code for a memory leak example I am running into. Can some one please advise me as to how to clean up this memory leak. This code can be compiled if you create a form and drop a button on it and then paste in the below code into the .pas file. Thanks in advance for any help that can be provided.

unit LeakForm;

interface

uses
  Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms,
  Dialogs, StdCtrls;
type PrintRecord = record
  PrintString1,
  PrintString2,
  PrintString3,
  PrintString4,
  PrintString5,
  PrintString6 : string;
  PrintFloat1,
  PrintFloat2,
  PrintFloat3  : Double;
end;
PrintPointer = ^PrintRecord;

type
  TForm1 = class(TForm)
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
    procedure FormCreate(Sender: TObject);
    procedure FormClose(Sender: TObject; var Action: TCloseAction);
  private
    { Private declarations }
    MyPrintLst : TList;
  public
    { Public declarations }
  end;

var
  Form1: TForm1;

implementation

{$R *.dfm}
procedure ClearTList(Var List : TList);
Var I, Count : Integer;
begin
  Count := list.Count - 1;
  For I := Count DownTo 0 Do
     Dispose(List[I]);
  List.Clear;
end;
procedure FreeTList(Var List : TList);
Var I, Count : Integer;
begin
  ClearTList(List);
  List.Free;
end;
procedure AddToPrintList(PrintList : TList;
                         Const MyStrings : Array of String;
                         Const MyDoubles : Array of Double);
var
PrintPtr : PrintPointer;
begin
New(PrintPtr);
IF High(MyStrings) >= 0 Then
   PrintPtr^.printString1 := MyStrings[0];
 Begin
   IF High(MyStrings) >= 1 Then
    Begin
      PrintPtr^.printString2 := MyStrings[1];
      IF High(MyStrings) >= 2 Then
       Begin
         PrintPtr^.printString3 := MyStrings[2];
         IF High(MyStrings) >= 3 Then
          Begin
            PrintPtr^.printString4 := MyStrings[3];
            IF High(MyStrings) >= 4 Then
               PrintPtr^.printString5 := MyStrings[4];
             Begin
               IF High(MyStrings) >= 5 Then
                  PrintPtr^.printString6 := MyStrings[5];
             End; {>=5}
          End; {>=4}
       End; {>=3}
    End; {>=2}
 End; {>=1}
IF High(MyDoubles) >= 0 Then
 Begin
   PrintPtr^.PrintFloat1 := MyDoubles[0];
   IF High(MyDoubles) >= 1 Then
    Begin
      PrintPtr^.PrintFloat2 := MyDoubles[1];
      IF High(MyDoubles) >= 2 Then
         PrintPtr^.PrintFloat3 := MyDoubles[2];
    End;
 End;
PrintList.add(PrintPtr);
end;
procedure TForm1.Button1Click(Sender: TObject);
Var EstReading : LongInt;
begin
EstReading := 0;
ClearTList(MyPrintLst);
AddToPrintList(MyPrintLst, ['Field1 Data','Field2 Data','Field3 Data','Field4 Data'],
                           [1,2,3,4]);
AddToPrintList(MyPrintLst, ['Field1 Data','Field2 Data','Field3 Data','Field4 Data'],
                           [5,6,7,8]);
AddToPrintList(MyPrintLst, ['Field1 Data','Field2 Data','Field3 Data','Field4 Data'],
                           [9,0,1,2]);
AddToPrintList(MyPrintLst, ['Field1 Data','Field2 Data','Field3 Data','Field4 Data'],
                           [3,4,5,6]);
end;

procedure TForm1.FormCreate(Sender: TObject);
begin
  MyPrintLst := TList.Create;
end;

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
FreeTList(MyPrintLst);
end;

end.

Upvotes: 1

Views: 1045

Answers (1)

David Heffernan
David Heffernan

Reputation: 613461

When you dispose each item, the runtime needs to know the type of the record. Because you are using a TList then each item is an untyped pointer. Therefore you need to cast the pointers to the item type so that the runtime knows the type, and knows how to dispose of the item.

Replace

Dispose(List[I]);

with

Dispose(PrintPointer(List[I]));

It's also a little odd that you pass the list as a var parameter and do not modify the reference. And the loop is quite odd too, running backwards for no reason, and the loop bounds are handled in a strange manner. I'd have these functions like this:

procedure ClearTList(List: TList);
Var
  I: Integer;
begin
  For I := 0 to List.Count - 1 Do
    Dispose(PrintPointer(List[I]));
  List.Clear;
end;

procedure FreeTList(List: TList);
begin
  ClearTList(List);
  List.Free;
end;

A more conventional naming convention would be:

type
  TPrintRecord = record
    ....
  end;
  PPrintRecord = ^TPrintRecord;

The form's OnClose event can be called multiple times if the form has the caHide action when closing. The correct event to pair with OnCreate is OnDestroy.

The complexity of the logic in AddToPrintList makes me believe that the data types can be designed in a better way. Arrays suggest themselves instead of individual numbered fields.

Without changing the types, you should at least avoid all that indentation, like this:

procedure AddToPrintList(PrintList: TList; const MyStrings: array of String;
  const MyDoubles: array of Double);
var
  I: Integer;
  Item: PPrintRecord;
  Str: string;
  Value: Double;
begin
  New(Item);
  PrintList.Add(Item);

  for I := 1 to Min(Length(MyStrings), 6) do
  begin
    Str := MyStrings[I - 1];
    case I of
    1:
      Item.PrintString1 := Str;
    2:
      Item.PrintString2 := Str;
    3:
      Item.PrintString3 := Str;
    4:
      Item.PrintString4 := Str;
    5:
      Item.PrintString5 := Str;
    6:
      Item.PrintString6 := Str;
    end;
  end;

  for I := 1 to Min(Length(MyDoubles), 3) do
  begin
    Value := MyDoubles[I - 1];
    case I of
    1:
      Item.PrintFloat1 := Value;
    2:
      Item.PrintFloat2 := Value;
    3:
      Item.PrintFloat3 := Value;
    end;
  end;
end;

Upvotes: 5

Related Questions