4

Consider this short Delphi procedure:

procedure TfrmXQuery.FieldListFillFromDefault;
var
  field_list: TStringList;
begin
  try
    if x <> '' then begin
      field_list := TStringList.Create;
      {do some stuff with field_list}
    end;
  finally
    if field_list <> NIL then 
    begin
      field_list.Free;
    end;
  end;
end;

When I run this in, Delphi 3, with x = '' so that field_list is never created,

  1. why is field_list <> NIL?
  2. are objects not initialized as NIL?
  3. if it's not NIL what is it?
  4. and if it's unassigned and not NIL how do I know whether or not to Free it? The Assigned function does not tell me: if Assigned(an_object) is equivalent to if an_object = NIL
Jonathan Elkins
  • 455
  • 5
  • 21
  • 1
    There is absolute no need to check for an assigned reference before Free. Free does this already and if assigned call Destroy. Thats why you should never call Destroy – Sir Rufo Mar 16 '13 at 03:53
  • 1
    Didn't the compiler warn you about this code? Never ignore a compiler diagnostic. – Rob Kennedy Mar 16 '13 at 04:34
  • 1
    This answer will be helpful to you: http://stackoverflow.com/questions/8548843/why-should-i-not-use-if-assigned-before-using-or-freeing-things/8550628#8550628 – David Heffernan Mar 16 '13 at 07:33
  • 1
    @SirRufo is absolutely right. `if Assigned(Foo) then Foo.Free` is essentially equivalent to `if Assigned(Foo) then if Assigned(Foo) then Foo.Destroy` because `X.Free` basically does `if Assigned(X) then X.Destroy`. – Andreas Rejbrand Oct 11 '18 at 12:05
  • @AndreasRejbrand but in the example in this question `Assigned(foo)` will return true, but foo is not initialized so `foo.Free` will fail. – Jonathan Elkins Oct 11 '18 at 12:20
  • 1
    @JonathanElkins: Indeed, that will fail spectacularly, but that is a different problem (much worse in practice, of course). – Andreas Rejbrand Oct 11 '18 at 12:33

2 Answers2

8

The problem is that if x = '', the finally happens anyway. Since field_list is only initialized when x <> '', it's a random memory location before that point, because it's an uninitialized local variable. The random value allows the field_list.free to be called, because it's not equal to nil. (Delphi doesn't initialize local variables (those declared within a function or procedure).)

var
  somevar: sometype;    
begin
  // at this point, somevar is just a chunk of memory that
  // holds whatever happens to be in that chunk
  somevar := nil;         // now somevar = a specific value you can test

  // other code
end;

You shouldn't have to test for <> nil (as others have pointed out in comments) if you structure your code correctly.

procedure TfrmXQuery.FieldListFillFromDefault;
var
  field_list  : TStringList;
begin
  if x <> '' then 
  begin
    field_list := TStringList.Create;
    try
      {do some stuff with field_list}
    finally
      field_list.Free;
    end;
  end;
end;

(If you turn on hints and warnings, the compiler would have told you that field_list may not have been initialized, which would have helped you solve this yourself.)

Ken White
  • 123,280
  • 14
  • 225
  • 444
  • There is no need for 'if field_list <> NIL'. `TObject.Free` already tests for this - in fact it is its entire purpose. All this does is say ' if field_list <> NIL then if field_list <> NIL then field_list.Destroy;' see http://docwiki.embarcadero.com/Libraries/XE2/en/System.TObject.Free – Gerry Coll Mar 16 '13 at 03:59
  • @Gerry: Nice catch. I missed removing them in both code snippets. Thanks. :-) – Ken White Mar 16 '13 at 04:17
  • 1
    "Delphi 3 didn't initialize local variables" No version of Delphi ever did. – David Heffernan Mar 16 '13 at 07:34
  • 4
    Moving the `if x <> ''` statement to the first line and encapsulate the following code within begin/end with a `try/finally` block would be more logical. – LU RD Mar 16 '13 at 09:02
  • 1
    David & LURD: Both points well taken. Answer revised accordingly. Note to self: Don't over-think things when you're posting late at night and getting tired. :-) – Ken White Mar 16 '13 at 14:21
  • Thank you guys. I can't believe I've been working with Delphi for so long and I have thought all this time that Objects were initialized to NIL everywhere. I will also turn on hints and pay special attention to "variable may not be initialized" messages. – Jonathan Elkins Mar 16 '13 at 21:14
-3

Answers to the questions:

  1. why is field_list <> NIL? Delphi does not initialize local objects. For more details see: Why doesn't object default to nil? and Are delphi variables initialized with a value by default?

  2. are objects not initialized as NIL? Global objects: Yes. Local objects: No.

  3. if it's not NIL what is it? Invalid pointers.

  4. and if it's unassigned and not NIL how do I know whether or not to Free it? You need to reorganize your code (see below). The Assigned function does not tell me: if Assigned(an_object) is equivalent to if an_object = NIL. Assigned tests for a nil (unassigned) pointer or procedural variable. <--from Delphi 3 documentation. Uninitialized local objects are not assigned NIL so Assigned(an_object) returns TRUE if an_object is local and has never been used (assigning NIL is using the object).

Because objects local to a procedure are not initialized to NIL, I have revised the code from the question assigning all local objects that are local to NIL. I do these assignments at the very beginning of the routine so that Free will not error out if the local objects are never created. I've also shown the error tracking code that was left out of the original question:

procedure TfrmXQuery.FieldListFillFromDefault;
var
  field_list: TStringList;
  some_other_object: TAnotherObject;
begin
  try
    try
      field_list := NIL;
      some_other_object := NIL;
      if x <> '' then begin
        field_list := TStringList.Create;
        {do some stuff with field_list}
      end;
      {...}
      some_other_object := TSomeOtherObject.Create;
      {...}
    except
      On E : Exception do begin
        ErrorTrackingRoutine(unit_name, name, 'FieldListFillFromDefault', E.message);
      end;
    end;
  finaly
    field_list.Free;
    some_other_object.Free;
  end;
end;

The entire routine is protected by try...except. If field_list or some_other_object are created it will be freed. They are assigned NIL at the very beginning so freeing them in the 'try...finally` block will not raise an error even if there is a run time error before they are created.

Jonathan Elkins
  • 455
  • 5
  • 21
  • 1
    That initialise to nil is wasted. Move the try/finally inside the if. Or better, convert the if into a guard right at the start: if x = '' then exit; Also, this does not answer question. – David Heffernan Mar 16 '13 at 22:45
  • Alternatively I could put the try..finally block around just the string list create and {do some stuff..} code but I like protecting the whole procedure; for example if it was a try..except block where I'm concerned that the if x test might fail. It's too bad that Delphi does not initialize local variable objects to nil. – Jonathan Elkins Mar 17 '13 at 06:33
  • 2
    Yes, I did read that text the first time. Using non-standard try/finally just makes the reader have to think harder about the code. Don't do that. – David Heffernan Mar 17 '13 at 07:53
  • What's non-standard about this use of try...finally? – Jonathan Elkins Mar 18 '13 at 20:10
  • 1
    What's not standard is that the `try` comes before the call to the constructor. The standard pattern is: `o := TObj.Create; try o.foo; finally o.Free; end;` Following that pattern makes it easier for the reader. So I'd use a guard clause to handle x = '' and then the standard try/finally. – David Heffernan Mar 18 '13 at 20:13
  • what if the constructors fails? – Jonathan Elkins Mar 18 '13 at 23:53
  • 1
    Then the assignment does not happen, the try block is not entered, and the exception bubbles up the stack – David Heffernan Mar 19 '13 at 00:04
  • I didn't show it in my code example but the except block tracks the call stack. If a constructor fails I want to know the call stack when it failed. – Jonathan Elkins Jul 01 '13 at 22:34
  • Alternately you might want to take some remedial action if the constructor fails. – Jonathan Elkins Jul 03 '13 at 22:40
  • I don't really understand why this answer is getting down voted without anyone addressing my last two comments. – Jonathan Elkins Jul 24 '18 at 17:11
  • 1
    There isn't an except block. No code in the answer does any of the things you describe in those comments. And it doesn't answer the question that was asked. Have another downvote. – David Heffernan Jul 24 '18 at 17:22
  • @DavidHeffernan noted. I have edited the answer according to your suggestions. – Jonathan Elkins Mar 12 '19 at 18:36