Liste mit TRects - Speicherleck?

[igel457]

Hallo, ich bin es mal wieder.

Ich habe eine Liste, in die man Rechtecke des Typs TRect einfügen kann (siehe unten).

Meine Frage ist nun, mache ich dies so richtig oder laufe ich womöglich Gefahr ein Speicherleck zu erzeugen?

Delphi-Quellcode:

procedure TRectList.Add(ARect: TRect);
var ar:PRect;
begin
  new(ar);
  ar^.Left := ARect.Left;
  ar^.Top := ARect.Top;
  ar^.Right := ARect.Right;
  ar^.Bottom := ARect.Bottom;
  inherited Add(ar);
end;

procedure TRectList.Clear;
var i:integer;
begin
  for I := 0 to Count - 1 do
  begin
    FreeMem(inherited Items[i]);
    Delete(i);
  end;
end;

function TRectList.GetItem(AIndex:integer):TRect;
begin
  result := PRect(inherited Items[AIndex])^;
end;

procedure TRectList.SetItem(AIndex:integer;AItem:TRect);
begin
  inherited Items[AIndex] := @AItem;
end;

Danke für eure Hilfe,
igel457

[jakobwenzel]

Setz einfach mal Application.ReportMemLeaksOnShutdown auf True, und du weißt, ob du memleaks hast.

[igel457]

Hm...

ReportMemLeaksOnShutdown finde ich leider nicht. Muss ich da noch irgendwas einbinden?

[DGL-luke]

Sollte das nicht ohne Application funktionieren?

[Hansa]

Du verwendest oft inherited. Z.B. hier :

  inherited Items[AIndex] := @AItem; Was ist z.B. das ? Von wo wird denn da was geerbt ?

[igel457]

Die Liste stammt von TList ab... habe ich vergessen zu sagen...

[SirThornberry]

dein Clear müsste irgendwann ein "Index out of Bounds" bringen weil du die Schleife vorwärts laufen lässt aber die Elemente der reihe nach löschst. Somit werden es immer weniger Items pro Durschlauf und die letzten Schleifendurchläufe klappen nicht mehr.
Deine Methode SetItems macht überhaupt keinen Sinn, denn AItem liegt auf dem Stack und ist nach verlassen der Methode nicht mehr gültig. Somit hängst du einen Pointer in deine Liste welcher auf eine Variable auf dem Stack zeigt welche früh oder Später überschrieben wird.

und in der Add-Methode kannst du anstelle von

Delphi-Quellcode:

ar^.Left := ARect.Left; 
ar^.Top := ARect.Top; 
ar^.Right := ARect.Right; 
ar^.Bottom := ARect.Bottom;

auch gleich schreiben
ar^ := ARect; gleiches kannst du auch in der Set-Methode machen, schließlich ist der Speicher schon durch Add reserviert wenn dem Index ein neuer Rect-Wert zugewiesen wird.
Genau da könnte auch dein Speicherleck sein. In der Add-Methode vorderst du mit "new" speicher an und hängst den Pointer auf diesen Speicher in die Liste. In der SetMethode überschreibst du diesen Pointer einfach ohne den alten Speicher frei zu geben. Aber wie gesagt macht es auch keinen Sinn den Pointer dort zu überschreiben. Sinvoller ist es das zu überschreiben wo der Pointer hin zeigt.

[igel457]

Danke für die Antworten, jetzt sieht das ganze so aus:

Delphi-Quellcode:

{TRectList}

procedure TRectList.Add(ARect: TRect);
var ar:PRect;
begin
  new(ar);
  ar^ := ARect;
  inherited Add(ar);
end;

procedure TRectList.Clear;
var i:integer;
begin
  while Count > 0 do
  begin
    FreeMem(inherited Items[i]);
    Delete(0);
  end;
end;

function TRectList.GetItem(AIndex:integer):TRect;
begin
  result := PRect(inherited Items[AIndex])^;
end;

procedure TRectList.SetItem(AIndex:integer;AItem:TRect);
var ar:PRect;
begin
  FreeMem(inherited Items[AIndex]);
  new(ar);
  ar^ := AItem;
  inherited Items[AIndex] := ar;
end;

Ist das so aktzeptabler?

Schonmal ein dickes Dankeschön.

[SirThornberry]

schau dir mal dein neues Clear an, dein "i" ist dort fehl am Platz.
Und in der SetMethode ist das freigeben überflüssig. Du gibst dort speicher frei und forderst sofort danach neuen Speicher an. Sinnvoller wäre es den einmal reservierten Speicher weiter zu nutzen.
PRect(inherited Items[AIndex])^ := AItem; und zu deinem Clear. Es geht auch so

Delphi-Quellcode:

var i: Integer;
begin
  for i := 0 to Count - 1 do
    Dispose(inherited Items[i]);
  inherited Clear();

Anstelle von Dispose kannst du natürlich auch weiter "FreeMem" nutzen.

[igel457]

Danke für die Hinweise, das mit dem "i" ist mir schon aufgefallen...

Delphi-Quellcode:

{TRectList}

procedure TRectList.Add(ARect: TRect);
var ar:PRect;
begin
  new(ar);
  ar^ := ARect;
  inherited Add(ar);
end;

procedure TRectList.Clear;
begin
  while Count > 0 do
  begin
    FreeMem(inherited Items[0]);
    Delete(0);
  end;
end;

function TRectList.GetItem(AIndex:integer):TRect;
begin
  result := PRect(inherited Items[AIndex])^;
end;

procedure TRectList.SetItem(AIndex:integer;AItem:TRect);
begin
  PRect(inherited Items[AIndex])^ := AItem;
end;

Ich arbeite so selten mit Pointern und Co, da bin ich mir nie so richtig sicher...

@SirThornberry
Was ist an deiner Clear Methode besser? Damit wird die Liste doch sogar zweimal durchlaufen...

Danke,
igel457