TStringList with objects

[MarkMLl]

Indeed. I would even go as far to advise only making and using classes, simply to keep it all simple and consistent. Learn to do one thing well.

Making and /instantiating/ classes.

Sorry, I'm not trying to be pedantic but there's far too much confusion in the terminology: the stringlist that OP's using is an instance of a class, and the object (optionally) associated with each string is an instance of a class.

The important point there is that TStringList.Objects contains instances of classes. In parallel with "class" there is also the deprecated "object", which is distinct and best avoided... at least until one has read, marked and digested the documentation together with the extended discussion at https://forum.lazarus.freepascal.org/index.php/topic,57040.0.html

MarkMLl

[Sieben]

But as @wp said, maybe you don't need to use FreeAndNil. I'm not interested into the debate of using FreeAndNil or not, but I only used it once in a program I wrote, and I really had the reason why FreeAndNil was needed. For all the other programs I wrote, using Free only, seem to run without any issue. So, I can sure to say 99% Free only is enough.

Most of the time because the vars just go out of scope, joining the quire invisible. But if they don't you'd better nil 'em.

This is only true for managed types and record and object instances that are located on the stack (or are global variables). Class instances and any other manual allocations need to be freed explicitely as well.

If you re-read the quotes you made this is exactly what we were talking about: of course you will have to free them, but you don't necessarily also have to nil the vars that used to hold (or point to) the class instances when they go out of scope anyway. I just added that you should as well nil when they don't, being a field in another class for example.

[Thaddy]

There are two anti patterns here:
1. Misusing a stringlist to store objects (a relic from the past)
2. The Free and Nil anti pattern.

Shame on all that are still under the illusion that both of those are sane programming.

[SymbolicFrank]

I do use TFPGMap and TFPGList, quite often. But if it is for a framework or template, I make all the constructors and methods I don't want used private. I will use TDictionary in the future.

And I do make custom classes all the time. Functions are for generic stuff only and not exposed. But inside those functions and classes I use whatever is easy and works.

[SymbolicFrank]

Btw, FreeAndNil is sometimes needed, because Free leaves a dangling pointer, without an obvious way to check the state that doesn't generate an exception. There are more reasons to free something than only it going out of scope. That's also why you have to nil the pointer if another object is the owner. And you cannot use FreeAndNill in that case, because you get the object itself, not the pointer. But in that case, you should remove the whole item (the string, for a TStringList) and let the owner clean it up. If you want to replace the object, first remove the previous one and then add the new one.

Then again, "fixing" Free would require either reference counting or limited garbage collection, so I do understand the problem. Still, that would prevent a lot of bugs.

[MarkMLl]

Also FreeAndNil() IIRC checks that its parameter is not nil before attempting to free it, which is a Good Thing.

However trying to remain focused on OP's use of TStringList, I think that not being able to do an indivisible free-and-nil via .Objects[] is an issue.

MarkMLl

[jcmontherock]

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

1. MyList.Objects[SomeValidIndex].Free;
2.  

What the difference between "Free" and "FreeInstance" ? Does it suppress the allocated memory for Objects ?

[Thaddy]

Btw, FreeAndNil is sometimes needed, because Free leaves a dangling pointer,

That is the basic mistake.
Dangling pointers are ALWAYS programmer error. Always.

[howardpc]

FreeAndNil is recommended in the FPC documentation here:

It is bad programming practice to call Destroy directly. It is better to call the Free method, because that one will check first if Self is different from Nil.
To clean up an instance and reset the reference to the instance, it is best to use the FreeAndNil function

[Handoko]

Pardon me if my English is limited. But these are what I get from that quote:
- Calling Free is better than call Destroy directly.
- FreeAndNil is for cleaning up instance and reset reference to the instance.
- None of them said FreeAndNil is better than Free only.

Challenge: Provide Me The Compile-able Demos

As I am the less experienced here, I am not qualify for arguing. But can anyone please provide real demos showing FreeAndNil is really needed? And if we change that FreeAndNil then the program will cause error.

I do not write large-scale-sized programs but I have been writing plenty of small and thousand lines of code programs. So far I only got 1 case why FreeAndNil is really required. But before I show you the code, I would be interested to see if anyone can provide me some.

FreeAndNil or not have been discussed for years but still have no a mutual agreement. So instead of just arguing, show me the code.

I will wait 1 week before I provide mine. But if till then still no one is able to provide a real demo then I will keep believing 99% Free only is enough.

[korba812]

In most cases it makes no difference what method you use. It is your choice and your preferences. Personally, I use FreeAndNil because problems like "use after free" can be easily detected - an AV exception will be raised.

[440bx]

But can anyone please provide real demos showing FreeAndNil is really needed? And if we change that FreeAndNil then the program will cause error.

FreeAndNil or not have been discussed for years but still have no a mutual agreement.

But if till then still no one is able to provide a real demo then I will keep believing 99% Free only is enough.

I normally stay away from discussions about OOP but, in this particular case, it's not just about OOP.

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.

As far as compile-able code, the subject "FreeAndNil" in the FPC Wiki, has an example that shows the problem with just Free.  It's also worth noting that there is code out there that tests if some class is Assigned before it proceeds to do something with it.  If the class/pointer is freed and not nil-ed, Assigned will return TRUE causing the rest of the code to access memory that is completely unrelated to the tested class.

Basically, whether in OOP or any other kind of programming, if a variable points to a dynamically allocated memory block and, the memory block is freed then the variable should be set to nil because whatever value it had got invalidated when the block got freed.

Think of it as variable "re-initialization".  The variable should be nil before a memory block is allocated to hold the class, it should be reset to nil when the block is freed.  That way the value/state of the variable mirrors the state of what it represents (as it should.)

[Handoko]

It is your choice and your preferences.

Very true. It is more about personal preference. Maybe I am sensitive, it seems to me the usage of FreeAndNil has been 'glorified'. People keep saying it's better. So, I might need to use it more too in the future but I need solid proofs that it is really better than simply Free.

As far as compile-able code, the subject "FreeAndNil" in the FPC Wiki, has an example that shows the problem with just Free.  It's also worth noting that there is code out there that tests if some class is Assigned before it proceeds to do something with it. If the class/pointer is freed and not nil-ed, Assigned will return TRUE causing the rest of the code to access memory that is completely unrelated to the tested class.

Yes, that was I what did. And my code won't run correctly without using FreeAndNil.  I didn't write the code to proof FreeAndNil, I really 'accidentally' encountered such case. But only once so far. So I later introspected, I thought maybe the problem was the code design.

I don't mean we shouldn't use FreeAndNil. I just want to proof 99% Free only is enough. Because the cases that FreeAndNil is required are really rare.

[balazsszekely]

@Handoko

Yes, that was I what did. And my code won't run correctly without using FreeAndNil.  I didn't write the code to proof FreeAndNil, I really 'accidentally' encountered such case. But only once so far. So I later introspected, I thought maybe the problem was the code design.

I don't mean we shouldn't use FreeAndNil. I just want to proof 99% Free only is enough. Because the cases that FreeAndNil is required are really rare.

Please teet the following scenario, but first make sure that Form2 is not in the Auto-Create Forms:

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

1. uses Unit2;
2.  
3. procedure TForm1.Button1Click(Sender: TObject);
4. begin
5.   Form2 := TForm2.Create(nil);
6.   try
7.     Form2.ShowModal;
8.   finally
9.     Form2.Free;
10.     //Form2 := nil;
11.   end;
12.  
13.   if Form2 <> nil then
14.     Form2.Caption := 'exception here without Form2 := nil';
15. end;

Comment/Uncomment  line: Form2 := nil; and see the difference.

[440bx]

I don't mean we shouldn't use FreeAndNil. I just want to proof 99% Free only is enough. Because the cases that FreeAndNil is required are really rare.

It's important to realize that it's not a matter of how rare something is needed.  It's simply good programming and that should be _continuous_.

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.

The value of a pointer should reflect the state of the block pointed to.  Whenever a block is freed, its pointer should be set to nil to reflect the fact that it no longer points to a valid block of memory.  It's actually somewhat rare to actually _need_ to reset the variable to nil after freeing the block but, it's the only genuinely correct thing to do because, if not, the value in the variable no longer reflects the state of the program and, sometimes that comes back to bite the programmer in undesirable areas