Disposing pointers to complex records

I have list of pointers to some complex records. Sometimes when I try disposing them I get invalid pointer operation error. I'm not really sure if I'm creating and disposing them properly. The record looks like this:

type
  PFILEDATA = ^TFILEDATA;
  TFILEDATA = record
    Description80: TFileType80;  // that's array[0..80] of WideChar
    pFullPath: PVeryLongPath;    // this is pointer to array of WideChar
    pNext: PFILE开发者_JAVA技巧DATA;            // this is pointer to the next TFILEDATA record
  end;

As I understand when I want a pointer to such record I need to initialize the pointer and the dynamic arrays like this:

function GimmeNewData(): PFILEDATA;
begin
  New(Result);
  New(Result^.pFullPath);
end;

Now to dispose of series of these records I wrote this:

procedure DisposeData(var pData: PFILEDATA);
var pNextData: PFILEDATA;
begin
  while pData^.pNext <> nil do begin
    pNextData := pData^.pNext;          // Save pointer to the next record
    Finalize(pData^.pFullPath);         // Free dynamic array
    Dispose(pData);                     // Free the record
    pData := pNextData;
  end;
  Finalize(pData^.pFullPath);
  Dispose(pData);
  pData := nil;
end;

When I run my program in the debug mode (F9) in the Delphi 2010 IDE something weird happens. When I step trough DisposeData code with F8 it appears that program skips Finalize(pData^.pFullPath) line and jumps to Dispose(pData). Is this normal? Also when Dispose(pData) is executed the Local variables window that displays contents of the pointers does not change. Does this mean that dispose fails?

Edit: PVeryLongPath is:

type
  TVeryLongPath = array of WideChar;
  PVeryLongPath = ^TVeryLongPath;

Edit2

So I create 2 TFILEDATA records then I dispose them. Then I create the same 2 records again. For some reason this time pNext in the second record is not nil. It points to the 1st record. Disposing this weird thing gets invalid pointer operation error. Randomly I have inserted pData^.pNext := nil in the DisposeData procedure. Now the code looks like this:

procedure DisposeData(var pData: PFILEDATA);
var pNextData: PFILEDATA;
begin
  while pData <> nil do begin
    pNextData := pData^.pNext;
    pData^.pNext := nil;          // <----
    Dispose(pData^.pFullPath);
    Dispose(pData);
    pData := pNextData;
  end;
end;

The error is gone. I'll try to change PVeryLongPath into TVeryLongPath.

---

First, if you free something, the contents of pointers to it do not change. That is why you don't see a change in the local variables display.

EDIT: declare pFullPath as TVeryLongPath. This is a reference type already, and you should not use a pointer to such a type. New() doesn't do what you think it does, in such a case.

It would probably be better if you declared it as UnicodeString, or if your Delphi doesn't have that, WideString.

If pFullPath is declared as a dynamic "array of WideChar", then you should not use New() on it. For dynamic arrays, use SetLength() and nothing else. Dispose() will properly dispose of all items in your record, so just do:

New(Result);
SetLength(Result^.pFullPath, size_you_need);

and later:

Dispose(pData);

In normal code, you should never have to call Finalize(). This is all taken care of by Dispose, as long as you pass a pointer of the correct type to Dispose().

FWIW, I would recommend this and this article of mine.

---

The fact that you accepted Serg's answer indicates that there is something wrong with your node creation code. Your comment to that answer confirms that.

I'm adding this as a new answer because the edits to the question significantly change it.

Linked list code should look like this:

var
  Head: PNode=nil;
  //this may be a global variable, or better, a field in a class, 
  //in which case it would be initialized to nil on creation

function AddNode(var Head: PNode): PNode;
begin
  New(Result);
  Result.Next := Head;
  Head := Result;
end;

Notice that we are adding the node to the head of the list. We don't need to initialize Next to nil anywhere because we always assign another node pointer to Next. That rule is important.

I've written this as a function which returns the new node. Since the new node is always added at the head this is somewhat redundant. Because you can ignore function return values it doesn't really do any harm.

Sometimes you may want to initialize the contents of the node when you add new nodes. For example:

function AddNode(var Head: PNode; const Caption: string): PNode;
begin
  New(Result);
  Result.Caption := Caption;
  Result.Next := Head;
  Head := Result;
end;

I much prefer this approach. Always make sure that your fields are initialized. If zero initialization is fine for you then you can use AllocMem to create your node.

Here's a more concrete example of using such a method:

type
  PNode = ^TNode;
  TNode = record
    Caption: string;
    Next: PNode;
  end;

procedure PopulateList(Items: TStrings);
var
  Item: string;
begin
  for Item in Items do
    AddNode(Head, Item);
end;

To destroy the list the code runs like this:

procedure DestroyList(var Head: PNode);
var
  Next: PNode;
begin
  while Assigned(Head) do begin
    Next := Head.Next;
    Dispose(Head);
    Head := Next;
  end;
end;

You can clearly see that this method can only return when Head is nil.

If you encapsulate your linked list in a class then you can make the head pointer a member of the class and avoid the need to pass it around.

The main point I would like to make is that manual memory allocation code is delicate. It is easy to make little mistakes in the details. In situations like that it pays to put the delicate code in helper functions or methods so you only need to write it once. Linked lists are a great example of a problem that loves to be solved with generics. You can write the memory management code once and re-use it for all sorts of different node types.

---

I recommend that you avoid using a dynamic array of WideChar which is not at all convenient to work with. Instead use string if you have Delphi 2009 or later, or WideString for earlier Delphi versions. Both of these are dynamic string types with WideChar elements. You can assign to them and Delphi deals with all the allocation.

So, assuming that you now have the following record:

TFILEDATA = record
  Description80: TFileType80;
  pFullPath: WideString; 
  pNext: PFILEDATA;          
end;

you can simplify things considerably.

function GimmeNewData(): PFILEDATA;
begin
  New(Result);
end;

procedure DisposeData(var pData: PFILEDATA);
var pNextData: PFILEDATA;
begin
  while pData <> nil do begin
    pNextData := pData^.pNext;          
    Dispose(pData);                     
    pData := pNextData;
  end;
end;

---

You should also initialize pNext field to nil - without it you will finally get access violation. Taking into account what was already said in the previous answers, you can change your code as

type
  TFileType80 = array[0..80] of WideChar;

  PFILEDATA = ^TFILEDATA;
  TFILEDATA = record
    Description80: TFileType80;
    FullPath: WideString;
    pNext: PFILEDATA;
  end;

function GimmeNewData: PFILEDATA;
begin
  New(Result);
  Result^.pNext:= nil;
end;

---

I think most of your problems are caused by the assumption that New() gives you memory that is zeroed out. I'm pretty sure (and I'm also sure someone will correct me if I'm wrong), but Delphi does not guarantee that that is the case. This can be rectified by changing your code to this:

function GimmeNewData(): PFILEDATA;
begin
  New(Result);
  ZeroMemory(Result, SizeOf(TFILEDATA));
end;

You should always either zero the memory you get allocated for a record, or at least fill all the fields with something else relevant. This behavior is different to objects, which are guaranteed to be zeroed on allocation.

Hope this helps.