Why isn't this dangerous, or is it?

[Renate Schaaf 64]

type
  TDoSomething = class
    Danger: string;
    constructor create;
    procedure DoSomething;
  end;

implementation
  ...
constructor TDoSomething.create;
begin
  Danger := 'Danger!';
end;

procedure TDoSomething.DoSomething;
begin
  ShowMessage('Doing something');
  Sleep(1000);
  ShowMessage('Done doing something');
  Free; //<-----
end;

procedure TForm2.Button1Click(Sender: TObject);
begin
  var
    DoSomething: TDoSomething := TDoSomething.create;
  DoSomething.DoSomething;
  // ShowMessage(DoSomething.Danger);
end;

initialization
ReportMemoryLeaksOnShutDown := true;
end.

I'm asking because I just noticed that I was doing something similar in my code , only it was a bit more hidden. It has worked without complaint for quite a while, so I was wondering why. My guess is, it's because the 'Free' is the very last thing I ask the class to do. In the destructor the class is still alive, after that the memory is freed, and as long as I don't do anything like the commented ShowMessage, that should be safe or no?

I am cleaning this up, of course, but I didn't want to miss the chance of learning something, I don't understand what's going on in the Delphi-source at this point all that well.

 

Renate

[Attila Kovacs 629]

27 minutes ago, Renate Schaaf said:

as long as I don't do anything like the commented ShowMessage, that should be safe or no?

After the "Free" the reference is a "dangling pointer". The ShowMessage could still work for centuries but only by luck as the memory where the class was is now free for other things.

So no, its not safe.

Try to use explicit Free's and you don't have to keep the implementation details in your head. Or keep it there and follow the rules.

[Renate Schaaf 64]

30 minutes ago, Attila Kovacs said:

After the "Free" the reference is a "dangling pointer".

That's why I don't use it :). 

33 minutes ago, Attila Kovacs said:

Try to use explicit Free's and you don't have to keep the implementation details in your head.

Good advice.

 

34 minutes ago, Attila Kovacs said:

Or keep it there and follow the rules.

So you are saying it is Ok (other than maintainance headaches) to keep it there on condition that one follows the rules? I'm saying this, because I remember reactions like "Shriek" when somebody posted code like this.

[Attila Kovacs 629]

3 minutes ago, Renate Schaaf said:

So you are saying it is Ok

Well, it was the last option 😉 As the example shows, this pattern hiding something from you. It's not your friend.

5 minutes ago, Renate Schaaf said:

other than maintainance headaches

Do not underestimate that. With time, you won't remember everything. I mean, a thing. You won't remember anything. 😉

 

 

[Pat Foley 51]

My frontal lobe Latency is one-two minutes L1 cache.  one to two weeks L2 cache every else is water under the bridge.   excepting a few grudges.    🙂   

[David Heffernan 2345]

1 hour ago, Renate Schaaf said:

So you are saying it is Ok

No I don't think so. Follow the actual rules. When you create an instance, destroy it in the same context that you created it. 

[corneliusdavid 214]

I don't think I've ever seen a procedure of a class free the instance of the class it's being called from. Everything in me is screaming DON'T DO IT!

 

This is very bad practice. The object code may still be in memory after the call to Free but it's no longer dedicated to that object and if, per chance, some other operation suddenly uses that available memory, you've got an access violation.

 

And to make an example that shows the problem of the dangling pointer @Attila Kovacspoint out:

procedure TForm1.Button1Click(Sender: TObject);
begin
  var
    DoSomething: TDoSomething := TDoSomething.create;

  DoSomething.DoSomething;

  if Assigned(DoSomething) then begin
    ShowMessage('DoSomething Freed but dangling reference still exists');
    DoSomething.DoSomething; // <--- ERROR!
  end;
end;

It's better to free the memory outside of the class itself in a try-finally block:

procedure TDoSomething.DoSomething;
begin
  ShowMessage('Doing something');
end;

procedure TForm1.Button2Click(Sender: TObject);
begin
  var
    DoSomething: TDoSomething;

  DoSomething := TDoSomething.create;
  try
    DoSomething.DoSomething;
  finally
    DoSomething.Free;
  end;
end;

This makes it obvious from the calling procedure (the button click event) that the object is freed which lessens the likelihood of using it again because it's right there in the procedure instead of hidden in the class itself.

Edited March 22, 2022 by corneliusdavid

[Renate Schaaf 64]

1 minute ago, David Heffernan said:

No I don't think so. Follow the actual rules.

I'm doing that, unless I'm too dim to notice. I just wanted to understand why it doesn't blow up in my face .. oh well.

[David Heffernan 2345]

7 minutes ago, Renate Schaaf said:

I'm doing that, unless I'm too dim to notice

You aren't. The code in @corneliusdavid post is what you should do. 

[corneliusdavid 214]

11 minutes ago, Renate Schaaf said:

I'm doing that, unless I'm too dim to notice

Create/Free are the calls that allocated/deallocate the memory for an object, an instance of TDoSomething in this case. In your original code, the call to Create in Button1Click but the Free happens in a procedure of TDoSomething--that's where the difference in context is.

 

12 minutes ago, Renate Schaaf said:

I just wanted to understand why it doesn't blow up in my face

The only reason it didn't blow up is because you don't reference the object (the DoSomething var) after the call to Free. Look at my first example code above where I point out where an ERROR could occur--that's what COULD have happened to you.

[Stefan Glienke 2002]

That is why FastMM and probably other memory managers have diagnostic features such as setting freed memory to a certain pattern. If you enable that your ShowMessage will most likely blow up because Danger would be an invalid string pointer.

[Renate Schaaf 64]

57 minutes ago, Stefan Glienke said:

setting freed memory to a certain pattern

Thanks, I didn't know that.

Otherwise my question (starting with why) has, as far as I am concerned, been answered by Attila, and I  wouldn't want to use anymore bandwidth for it.

[bobD 6]

On 3/22/2022 at 4:17 PM, corneliusdavid said:

 

It's better to free the memory outside of the class itself in a try-finally block:

 

Absolutely. The inclusion of Free in the instance method is essentially a side-effect of the method and constitutes a violation of the SRP. What if I want to call DoSomething twice?

 

Also, as a matter of consistency I personally always call the inherited constructor--even when descending from TObject directly. I have no guarantee that TObject.Create will always be an empty method.

[DelphiUdIT 176]

In general, when I create a new class, a new feature or other, I always think about the possibility that the class is derived (inheritance, polymorphism) and therefore writing a code like the one indicated I would not write it even if it were possible without side effects.

 

Bye