View Issue Details

IDProjectCategoryView StatusLast Update
0036115FPCRTLpublic2019-10-10 18:44
ReporterThaddy de KoningAssigned ToSven Barth 
PrioritynormalSeverityminorReproducibilityalways
Status assignedResolutionreopened 
Product Version3.3.1Product Build 
Target VersionFixed in Version 
Summary0036115: code from "Feature request: type safe FreeAndNil" causes Internal error when added to sysutils
DescriptionGhist: https://wiert.me/2017/12/21/another-case-against-freeandnil/ and much older discussions

In FreePascal it is possible to write a fully type safe FreeAndNil using a generic procedure.

This would remove some of the objections against it.

Like adding:
  procedure FreeAndNil<T:class>(var obj:T);
  var
    temp: T;
  begin
    temp := obj;
    obj := nil;
    temp.free;
  end;
  
Or in objfpc mode:

  generic procedure FreeAndNil<T:class>(var obj:T);
  var
    temp: T;
  begin
    temp := obj;
    obj := nil;
    temp.free;
  end;

Steps To ReproduceTry one of the examples where FreeAndNil is your enemy.
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget-
Attached Files

Activities

Michael Van Canneyt

2019-09-30 12:03

administrator   ~0118210

Assigned to Sven. Adding this to sysutils causes an internal compiler error.

Michael Van Canneyt

2019-09-30 12:04

administrator   ~0118211

Note that we only consider adding this as a PR measure, as we don't think it actually solves any problem.

Thaddy de Koning

2019-09-30 14:09

reporter   ~0118213

Last edited: 2019-09-30 14:12

View 3 revisions

If it is only PR -which I don't think it is - I withdraw and plz close.

I will not pursue this if the following is considered:

As it stands, the current implementation of FreeAndNil can be called on anything, like in a List of pointers, Tstringlist.Objects or even a TControl.tag.
FreeAndNil can be called on literally anything! since the parameter is untyped var.
Agree, those are by current standards common mistakes but I see them all too often.
That's aside from some of the issues mentioned by Allen Bauer and Jeroen Pluimers.

References:
https://blog.therealoracleatdelphi.com/2010/02/a-case-when-freeandnil-is-your-enemy_16.html
https://blog.therealoracleatdelphi.com/2010/02/a-case-against-freeandnil_5.html
http://nickhodges.com/post/Using-FreeAndNil.aspx

As well as the link in the original report.

Michael Van Canneyt

2019-09-30 14:19

administrator   ~0118214

Closing as per OP's request.

Thaddy de Koning

2019-09-30 14:22

reporter   ~0118216

Sorry to re-open:
There is a strong anti-pattern here: https://eurekalog.blogspot.com/2009/04/why-should-you-always-use-freeandnil_28.html

If the available pointers - including the above - still not satisfy developers, close again.

Sven Barth

2019-10-03 14:51

manager   ~0118275

I'm reopening this, because adding this code nevertheless points to a bug in the compiler that must be fixed and I want to keep this as a reminder.

Also an argument against FreeAndNil from Nick Hodges: http://www.nickhodges.com/post/Using-FreeAndNil.aspx

Further discussion about this should be moved to the forum or the mailing lists however.

Marco van de Voort

2019-10-04 13:25

manager   ~0118319

Last edited: 2019-10-04 13:26

View 2 revisions

Updated title to reflect current position on report.

MHO: I'm more on Nick Hodges with this, and see no reason for overuse of freeandnil.

Benito van der Zander

2019-10-04 16:34

reporter   ~0118324

I hope fpc is smart enough to notice that the assembly code of that function is the same for every type and only generate one copy of function in the final program.

Sven Barth

2019-10-05 10:34

manager   ~0118344

Currently it's not able to detect that. It's even worse when you have the same specialization in different, unrelated parts of a project, cause then the same specialization for even the same type parameters will be placed in the binary multiple times. I already have some ideas to address this, but the topic is not easy.

Benito van der Zander

2019-10-05 12:38

reporter   ~0118346

Then this new code is a bad idea

Michael Van Canneyt

2019-10-05 12:57

administrator   ~0118347

Sven reopened the bugreport not because he will actually add the new call, but because an attempt to add it revealed a bug in the compiler.

As said originally, the idea to add it was purely a PR move.
I agree that if the result is that the FreeAndNil call is duplicated for every class that it is used for, it is even a very bad idea.

Akira1364

2019-10-10 17:22

reporter   ~0118466

Last edited: 2019-10-10 17:27

View 4 revisions

As far as the assembly code duplication, while that may be the case, presumably the generic FreeAndNil would not actually be used *in* the FPC codebase itself and so would not have any effect whatsoever on binary size by default.

It's no different at all from literally any other generic function or procedure that might ever be added somewhere in the RTL.

For example, currently there is a generic version of IfThen available in SysUtils, which obviously does not cause any bloat issues simply by existing.

In many cases having a generic version of a function or procedure available is actually likely to reduce bloat, I'd say, because they do not generate any assembly code *at all* unless you actively use them in your program, compared to overloaded non-generic functions and procedures (such as the three non-generic overloads of IfThen in math.pp) which *always* generate assembly code regardless of if they're ever used or not.

So in general, I don't think the bloat argument really makes any sense as far as RTL routines, because if they're not actually called *in* the RTL then they literally take up zero space.

Michael Van Canneyt

2019-10-10 17:30

administrator   ~0118467

IMHO the generic version would be used since it would be selected through automatic type inference.
So it would end up being copied all over the place.

Akira1364

2019-10-10 18:26

reporter   ~0118472

Last edited: 2019-10-10 18:44

View 3 revisions

I suppose that might be true, although I don't quite recall exactly how Ryan's patch would handle that particular scenario (where one of the options takes untyped var.)

At the same time I think that would possibly have the benefit of directly exposing "wrong" uses of FreeAndNil though (as in where Obj was not in fact a class at all.)

On the other hand, that could also be avoided completely by just calling the generic version something slightly different, like "FreeAndNilG" or whatever, since there's no real reason that it has to have exactly the same name in the first place.

Issue History

Date Modified Username Field Change
2019-09-29 14:45 Thaddy de Koning New Issue
2019-09-29 14:57 Michael Van Canneyt Assigned To => Michael Van Canneyt
2019-09-29 14:57 Michael Van Canneyt Status new => assigned
2019-09-30 12:02 Michael Van Canneyt Assigned To Michael Van Canneyt => Sven Barth
2019-09-30 12:03 Michael Van Canneyt Note Added: 0118210
2019-09-30 12:04 Michael Van Canneyt Note Added: 0118211
2019-09-30 14:09 Thaddy de Koning Note Added: 0118213
2019-09-30 14:11 Thaddy de Koning Note Edited: 0118213 View Revisions
2019-09-30 14:12 Thaddy de Koning Note Edited: 0118213 View Revisions
2019-09-30 14:19 Michael Van Canneyt Status assigned => resolved
2019-09-30 14:19 Michael Van Canneyt Resolution open => no change required
2019-09-30 14:19 Michael Van Canneyt FPCTarget => -
2019-09-30 14:19 Michael Van Canneyt Note Added: 0118214
2019-09-30 14:22 Thaddy de Koning Status resolved => feedback
2019-09-30 14:22 Thaddy de Koning Resolution no change required => reopened
2019-09-30 14:22 Thaddy de Koning Note Added: 0118216
2019-09-30 14:26 Michael Van Canneyt Status feedback => resolved
2019-09-30 14:26 Michael Van Canneyt Resolution reopened => no change required
2019-09-30 14:26 Michael Van Canneyt Assigned To Sven Barth => Michael Van Canneyt
2019-10-03 14:51 Sven Barth Assigned To Michael Van Canneyt => Sven Barth
2019-10-03 14:51 Sven Barth Status resolved => feedback
2019-10-03 14:51 Sven Barth Resolution no change required => reopened
2019-10-03 14:51 Sven Barth Note Added: 0118275
2019-10-03 14:51 Sven Barth Status feedback => assigned
2019-10-04 13:25 Marco van de Voort Summary Feature request: type safe FreeAndNil => code from "Feature request: type safe FreeAndNil" causes Internal error when added to sysutils
2019-10-04 13:25 Marco van de Voort Note Added: 0118319
2019-10-04 13:26 Marco van de Voort Note Edited: 0118319 View Revisions
2019-10-04 16:34 Benito van der Zander Note Added: 0118324
2019-10-05 10:34 Sven Barth Note Added: 0118344
2019-10-05 12:38 Benito van der Zander Note Added: 0118346
2019-10-05 12:57 Michael Van Canneyt Note Added: 0118347
2019-10-10 17:22 Akira1364 Note Added: 0118466
2019-10-10 17:24 Akira1364 Note Edited: 0118466 View Revisions
2019-10-10 17:26 Akira1364 Note Edited: 0118466 View Revisions
2019-10-10 17:27 Akira1364 Note Edited: 0118466 View Revisions
2019-10-10 17:30 Michael Van Canneyt Note Added: 0118467
2019-10-10 18:26 Akira1364 Note Added: 0118472
2019-10-10 18:27 Akira1364 Note Edited: 0118472 View Revisions
2019-10-10 18:44 Akira1364 Note Edited: 0118472 View Revisions