4

I have the following Delphi code:

destructor TXX_XXXX.Destroy;
var
i: Integer;
begin
  if Assigned(Allocations) then
  begin
    for i:=0 to (Allocations.Count - 1) do 
    begin
      try
      TXX_ALOC(Allocations.Items[i]).Free;
      except on Ex:Exception do
      begin
        OutputDebugString(PChar('Exception Error Message '+ Ex.Message));
      end;
      end;
    end;

        // Above code works well - no exception

        try
    FreeAndNil(Allocations); {Exception Here}
    except on E:Exception do
    begin
      OutputDebugString(PChar('Exception in xxxxxxxxx.pas'+E.Message));
    end;
    end;
  end;
  inherited;
end;

Access violation at address 4003AB4 in module 'Vcl50.bpl'. Read of address 2980BFFC

I know the access violation usually caused by

  1. free some object which has been freed before
  2. use some object without initialization

But here before I do the free, I checked Allocations is assigned. If I discard the exception handling, my application will throw a something is wrong error. Allocations is a TObjectList, if it is an array - I will doubt I didn't assign a length to the array, but it is a TObjectList.

Thanks a lot!

Lars Truijens
  • 42,837
  • 6
  • 126
  • 143
spspli
  • 3,128
  • 11
  • 48
  • 75
  • 5
    "But here before I do the free, I checked Allocations is assigned"...note that this only helps if you not only call `Free` but also set the reference to `nil` explicitly (or better use `FreeAndNil`). `Free` does not set the reference to `nil` and that is what `Assigned` checks! – jpfollenius Aug 16 '11 at 13:58

3 Answers3

18

A TObjectList usually takes care of destroying its content. Don't free your objects in this case. This will lead to an access violation when freeing the TObjectList because it tries to free the contained objects again.

This behavior of the object list can be controlled in its constructor:

constructor TObjectList.Create(AOwnsObjects: Boolean);

Use this one to specify if you want the list to own its content (means: it takes care of destroying an item when it is removed from the list or the list is destroyed) or not. The constructor without parameter (which you probably used) sets this to true.

You probably just want a list like TList but for storing objects. If that's the case then create your list like this:

Allocations:= TObjectList.Create(False);

But if you want the auto-destruction behavior then just remove the for-loop. The object list will destroy your TXX_ALOC objects.

Heinrich Ulbricht
  • 10,064
  • 4
  • 54
  • 85
7

Generally, when clearing lists you want to loop from end to start ie

for i := (Allocations.Count - 1) downto 0 do begin
   Delete(Allocations.Items[i]);
end

but in case of TObjectList, if the list owns objects (which it does by default) you shouldn't free them before destroying the container as the list will do it for you. In the code above, if the list owns the objects then calling Delete also frees the object.

ain
  • 22,394
  • 3
  • 54
  • 74
  • Since he doesn't remove them it's not that critical. But usually you do. – Heinrich Ulbricht Aug 16 '11 at 13:58
  • 1
    Clearing the list <> freeing the elements in the list – jpfollenius Aug 16 '11 at 14:37
  • Generally yes. However, if there is some more complicated ownership model in place then destroying the obj might cause the list to remove the obj from itself. And looping towards zero is slightly optimal too, so IMHO it is good idea to use it anyway. But I changed the example code to use `Delete` now. – ain Aug 16 '11 at 17:22
  • I haven't met a more complicated ownership model so very hard to understand it here - why use Delete instead of Free. – spspli Aug 16 '11 at 21:48
  • 1
    @spspli If TObjectList owns the objects it contains, then calling `Delete` would **remove** the item from the list **and free** it. But since the count of objects in the list changed you have to loop downto 0, otherwise you would get error as the for loop index is only calculated once, in the beginning of the loop. – ain Aug 16 '11 at 22:18
3

There are 2 options for your ...

1) Create the TObjectList wiht the aOwnsObjects set to true if you want Delphi to automatically free the objects when they get removed from the ObjectList. And in your Destructor simple FreeAndNil the ObjectList itself. This will then remove all objects from the list and free them automatically. Since freeing the ObjectList will automatically free the objects contained in that list, Your code could then look like :

destructor TXX_XXXX.Destroy;
begin
  FreeAndNil( Allocations ); //
  inherited;
end;

2) Create the TObjectList wiht the aOwnsObjects set to False, in which case you will have to take care of freeing the objects in the list yourself. Your code could then look something like this :

destructor TXX_XXXX.Destroy;
var
  i       : Integer;
  oObject : TObject;
begin
  if Assigned(Allocations) then
  begin
    for i:= Pred( Allocations.Count ) downto 0 do
    begin
      // Get a reference to the Object
      oObject := TXX_ALOC(Allocations.Items[i]);
      // Remove the object from the object list
      Allocations.Delete( i );
      // Free the Object
      FreeAndNil( oObject );
    end;
  end;

  FreeAndNil( Allocations );
  inherited;
end;
Stefaan
  • 492
  • 4
  • 19
  • 1
    The `FreeAndNil( oObject );` in the loop is "overkill" as youre nil'ing an local var. Just `oObject.Free` would do. I'm sure Rudy will teach you shortly :) – ain Aug 16 '11 at 14:21
  • Using `FreeAndNil` even for local variables does no harm IMHO and it has the benefit that one does not have to think about which one to use every time. – jpfollenius Aug 16 '11 at 14:39
  • See here for the discussion of `Free` vs `FreeAndNil`: http://stackoverflow.com/questions/3159376/which-is-preferable-free-or-freeandnil – jpfollenius Aug 16 '11 at 14:56