TStringList with objects

[MarkMLl]

The "clean" (for lack of a better word) way is to use FreeAndNil.  The reason is very simple, a class is a pointer to a heap memory block.  Once the memory block has been freed, it should not be accessed again.  If the class/pointer has not been nil-ed then it is possible to dereference the pointer after the memory has been freed, which is a programming error but, it often won't be visible immediately.  if the pointer is nil-ed then any subsequent de-reference will cause an access violation (as it should) revealing the programming error right then and there.

I'd be happier if it were documented as being indivisible.

Since it's not, there's the potential for an avoidable race condition when the pointer is first inspected.

MarkMLl

[wp]

I don't condemn FreeAndNil, but I try to avoid it as much as possible because it can hide bugs. Everybody thinks exceptions are a bad thing - no they are here to help you to find bugs. Checking freed pointers for nil is a habit which prevents exceptions - but along with automatic nilling deallocated pointers this prevents you from seeing the bug.

Here is an example:

Suppose a form with a TStringList, FList.

There is a button which creates the stringlist and reads a data file into it. But you make a mistake and destroy the StringList immediately afterwards, and because you have been told to use FreeAndNil you are used to typing FreeAndNil for destroying it:

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button1Click(Sender: TObject);
2. begin
3.   FList := TStringList.Create;
4.   FList.LoadFromFile('data.txt');
5.   FreeAndNil(FList);    // better: FList.Free;
6. end;

There is another button  which is supposed to analyze the contents of the stringlist, maybe for duplicate lines. Because for some reasons the click handler of this button performs some other things the button can be clicked before the FList has been created, and you decide to simply check FList for not being nil:

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button2Click(Sender: TObject);
2. begin
3.   DoSomethingElse;
4.   if FList <> nil then
5.     FindDuplicateLines(FList);
6. end;

Running this program you will notice no duplicates lines - never. Unless you know for sure that the test file does contain duplicate lines you will not notice that the analysis is not correct.

The problem, of course, is the destruction of the stringlist after creating it (Button1Click). If you were not used to typing FreeAndNil without thinking you'd have typed "FList.Free". And this would have caused an exception in your tests with ANY file.

So, my message is: Never type FreeAndNil without thinking. I agree with Handoko above that calling .Free instead of FreeAndNil is better in most cases.

[SymbolicFrank]

The main problem with Free is multiple pointers to the same object. Yes, if you only have one pointer and the object goes out of scope, Free is fine. But if you don't like globals, you pass pointers around. You probably have a class somewhere that is the owner of that object, like that TStringList. You need to have one owner for each object instance, to decide who is going to Free it.

So, we have an object, with an owner, that hands out pointers to that object. Not to the pointer to that object it manages, but directly. If one of the parties that have one of those pointers free the object, all those pointers become invalid. FreeAndNil isn't going to help there, because it will only nil the pointer that is used to Free the object. And the only way to test if your pointer is still valid is accessing the object: if an access exception is raised, it wasn't. Lots of try .. except.

So, how do you fix that? Thaddy would say: if someone else than the owner frees it, you're doing it wrong. And pointers passed are one-use only: don't store them or pass them along! Which is fair enough, and doable if you're the only programmer. But it's not very bullet-proof.

A better option would be: hand out pointers to the pointer. That way, there is only one master pointer, which is dereferenced by the other pointers. FreeAndNil that, and you're done. This is the idea behind smart pointers.

You could also store all references to the object, possibly in a linked list that is part of TObject. Like reverse reference counting. On Free, you start by nilling all those pointers. This would require changing the compiler.

And lastly, garbage collection. Keep a list of all pointers used and nil the ones that pointed to that object after each Free. This is the brute-force method.

[440bx]

I don't condemn FreeAndNil, but I try to avoid it as much as possible because it can hide bugs. Everybody thinks exceptions are a bad thing - no they are here to help you to find bugs. Checking freed pointers for nil is a habit which prevents exceptions - but along with automatic nilling deallocated pointers this prevents you from seeing the bug.

On the contrary, if the pointer is nil and it's dereferenced, there will be an exception (access violation.)  Thus clearly revealing the presence of a bug.

I agree that checking freed pointers before doing something with it is a bad habit.  The programmer should know _beforehand_ whether or not the pointer is nil.  IOW, checking that shouldn't be necessary in a well structured program.

Just for the record and, this shouldn't be news to anyone who has read my posts, I am one of those who is strongly against the casual use of exceptions.  Exceptions should be used for things that, as their name implies, are exceptional, not for trivial error handling resulting in cross stack frame gotos whose destination is, more often than not, unpredictable.

if a pointer isn't pointing to a valid memory block, it should be nil and, the reason is obvious, because it doesn't point to a valid memory block.

[korba812]

So, my message is: Never type FreeAndNil without thinking. I agree with Handoko above that calling .Free instead of FreeAndNil is better in most cases.

But by modifying your example you can show that FreeAndNil is useful:

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button1Click(Sender: TObject);
2. begin
3.   FList := TStringList.Create;
4.   FList.LoadFromFile('data.txt');
5.   FList.Free;
6. end;

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button2Click(Sender: TObject);
2. begin
3.   DoSomethingElse;
4.   if FList <> nil then   // Do we still have a valid reference to the class? Nobody knows...
5.     FindDuplicateLines(FList);
6. end;

[wp]

But by modifying your example you can show that FreeAndNil is useful:

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button1Click(Sender: TObject);
2. begin
3.   FList := TStringList.Create;
4.   FList.LoadFromFile('data.txt');
5.   FList.Free;
6. end;

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button2Click(Sender: TObject);
2. begin
3.   DoSomethingElse;
4.   if FList <> nil then   // Do we still have a valid reference to the class? Nobody knows...
5.     FindDuplicateLines(FList);
6. end;

It is always difficult it create useful and non-trivial examples... But as I wrote the example is constructed such that the Button1Click contains a bug, namely destroying the list immediately after loading the file which should be done somewhere else (certainly in FormDestroy or in some other button click). Assuming that it is a bug, nilling the instance by FreeAndNil makes it even worse because now the bug is hidden and hard to find, rather than raising an easily observed exception.

[SymbolicFrank]

It is interesting, that most languages with garbage collection assume that object instances exist until they go out of scope everywhere. So, what are you supposed to do if you want to replace that instance with another one? I do that often.

I mean, are you going to call Destroy and Create a new one with the same pointer? How do you make sure the other pointers don't keep pointing to the depreciated instance?

[Handoko]

@GetMem, @korba812

Thank you for providing those codes. But I think I need to rephrase what I said. The codes you showed are specially written to show bad programming practice. I would say those were the programmer fault. Why do still use that variable when you know for sure you have free it?

Okay, maybe this is what I need. Provide me the demos showing in what cases we need to reuse the free pointer when we know we already free them. Or better this way: Do you have any codes which have written correctly and they use FreeAndNil? And if you use the Find and Replace tool to replace those FreeAndNil to Free only, then the program stop working correctly.

I need real world cases. Not some imaginary cases or some lines specially written to show bad programming practices.

@440bx

For instance, declaring the initial value of a variable in a function is actually rarely needed because the common case is that the variables will be assigned some value in the code (at least they should) but, by initializing them in the declaration you know  what their initial value is, instead of being whatever they overlay on the stack, because of that, if something isn't working quite right, it will usually be much easier to find and also, the behavior will almost always be reproducible because the variable are always initialized to a specific value.  Whereas if one or more variables are left uninitialized, they'll have whatever values happened to be on the stack causing the behavior to often vary, making the bug harder to find.

Nice explanation. Using FreeAndNil is recommended because it is a good practice even it's rarely needed. Okay, I got your point. But I am more interested to see how frequent or rare is really needed.

So, my message is: Never type FreeAndNil without thinking.

+1
It is good to know why we use it instead of using it blindly without knowing the reason.

I remember some said misusing FreeAndNil may make the code harder to debug because it hides the real issue ... or maybe something like that. Can anyone explain in more detail?

[Zoran]

I remember some said misusing FreeAndNil may make the code harder to debug because it hides the real issue ... or maybe something like that. Can anyone explain in more detail?

https://community.embarcadero.com/blogs/entry/a-case-against-freeandnil-38910
https://blogs.embarcadero.com/a-case-when-freeandnil-is-your-enemy/
https://wiert.me/2017/12/21/another-case-against-freeandnil/
https://www.delphitools.info/2010/02/06/dont-abuse-freeandnil-anymore/
https://stackoverflow.com/questions/3510179/why-should-i-use-free-and-not-freeandnil-in-a-destructor

[440bx]

But I am more interested to see how frequent or rare is really needed.

That will vary from program to program depending on how many blocks the program dynamically allocates, frees and re-uses.

I remember some said misusing FreeAndNil may make the code harder to debug because it hides the real issue ... or maybe something like that. Can anyone explain in more detail?

I am of the opposite opinion.  FreeAndNil should always make the code easier to debug because, if a pointer is nil then the reason why the de-reference is failing is completely obvious, whereas if the pointer has a value that points somewhere in memory, it will be much more difficult to figure out that it shouldn't be pointing there and figuring out what the values mean will be an exercise in futility because the pointer has no business pointing there.

A non-nil pointer that points to a freed block is a wolf disguised as a sheep.  (bad news for anyone who takes it for granted that sheep are harmless.)

[SymbolicFrank]

This shows the problem:

Code: Pascal  [Select][+][-]

1. program Project1;
2.  
3. uses
4.   Sysutils, Classes, Variants;
5.  
6. type
7.   TRecord = class
8.   public
9.     Fields: array of Variant;
10.   end;
11.  
12.   { TTable }
13.  
14.   TTable = class
15.   private
16.     MyRecord: TRecord;
17.     function GetNext: TRecord;
18.   public
19.     constructor Create(Number: Cardinal);
20.     procedure Next;
21.     procedure Post;
22.     property Current: TRecord read MyRecord;
23.   end;
24.  
25. { TMyClass }
26.  
27. function TTable.GetNext: TRecord;
28. begin
29.   Result := nil;
30.  
31.   // Now go and fetch the next record
32.   // Result := ...
33. end;
34.  
35. constructor TTable.Create(Number: Cardinal);
36. var
37.   i: Integer;
38. begin
39.   Setlength(MyRecord.Fields, Number);
40.   for i := 0 to Number - 1 do MyRecord.Fields[i] := NULL;
41. end;
42.  
43. procedure TTable.Next;
44. begin
45.   MyRecord.Free;
46.   MyRecord := GetNext;
47. end;
48.  
49. procedure TTable.Post;
50. begin
51.   // Write Myrecord to the database
52. end;
53.  
54. var
55.   MyTable: TTable;
56.   MyRecord: TRecord;
57. begin
58.   MyTable := TTable.Create(10);
59.   MyRecord := MyTable.Current;
60.   MyTable.Next;
61.   MyRecord.Fields[1] := 'Bla';
62.   MyTable.Post;
63. end.

It's obvious here, but normally that is not so transparent.

[balazsszekely]

@Handoko

Thank you for providing those codes. But I think I need to rephrase what I said. The codes you showed are specially written to show bad programming practice. I would say those were the programmer fault. Why do still use that variable when you know for sure you have free it?

I agree that you can consider my example a bad programming practice, but I had to come up with something simple to illustrate the issue. In other, not so trivial cases it's very useful to set a particular object to nil.

[wp]

I remember some said misusing FreeAndNil may make the code harder to debug because it hides the real issue ... or maybe something like that. Can anyone explain in more detail?

Did you see my example in reply #46?

[Thaddy]

A TStringlist with objects is a MAP/DICTIONARY pattern as I wrote. So use a map/dictionary. Not the legacy code. That is core.

[dseligo]

There is another button  which is supposed to analyze the contents of the stringlist, maybe for duplicate lines. Because for some reasons the click handler of this button performs some other things the button can be clicked before the FList has been created, and you decide to simply check FList for not being nil:

Instead of using FList only if it is not nil, you should alert yourself or user that FList is not valid:

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button2Click(Sender: TObject);
2. begin
3.   DoSomethingElse;
4.   if FList = nil then
5.     Raise Exception.Create('FList is nil and it shouldn''t be.');
6.   FindDuplicateLines(FList);
7. end;