views:

456

answers:

4

I have a method which constructs an object, calls an Execute method, and frees the object. The type of object is determined by a TClass descendant passed into the method. Note this is Delphi for Win32 I am talking about, not .NET.

Edit: I should point out that this is Delphi 2006, as it has been noted in answers below that in future versions the NewInstance call may not be required. In my case, however, it is required. As such, I would imagine the answer to my question (is it safe? and does CreateForm() have a potential leak) would need to be answered on the basis that this is Delphi 2006

Edit#2: seems that the solutions given for D2007 & D2009 do in fact work for D2006. I must have picked up the "NewInstance" habit from an earlier version of Delphi...

function TPageClassFactory.TryExecute(ScrnClass: TCustomPageClass): boolean;
//TCustomPageClass = class of TCustomPage
var
  ScrnObj: TCustomPage; //TCustomPage defines an abstract Execute() method
begin
  Result := FALSE; //default
  ScrnObj := TCustomPage(ScrnClass.NewInstance); //instantiate
  try
    ScrnObj.Create(Self);  //NB: Create() and Execute() are *virtual* methods
    ScrnObj.Execute;       
  finally
    FreeAndNil(ScrnObj);
  end;
  Result := TRUE;
end;

What I want to know is whether this is safe - what will happen here if Create() raises an exception?

Looking at a similar example, from Forms.pas.TApplication.CreateForm(), a different approach has been taken to exception handling (I've cut out the irrelevant bits below):

procedure TApplication.CreateForm(InstanceClass: TComponentClass; var Reference);
var
  Instance: TComponent;
begin
  Instance := TComponent(InstanceClass.NewInstance);
  TComponent(Reference) := Instance;
  try
    Instance.Create(Self);
  except
    TComponent(Reference) := nil;
    raise;
  end;
end;

In the Forms.pas method, does this mean that memory is leaked when an exception occurs in the Create() method? My understanding was that InstanceClass.NewInstance allocated memory, thus in this case the memory is not being deallocated/released/freed?

+10  A: 

You should put the create out of the try finally block.

But a better solution is:

type 
  TMyClass = class ()
  public
    constructor Create(...); virtual;
    fucntion Execute: Boolean; virtual;
  end;
  TMyClassClass = class of TMyClass;


procedure CreateExecute(const AClass: TMyClassClass): Boolean;
var
  theclass : TMyClass;
begin
  theclass := AClass.Create;
  try
    Result := theclass.Execute;
  finally
    theclass.Free;
  end;
end;
Gamecat
I am using Delphi 2006 - my understanding of why is a little hazy, but NewInstance is required either because AClass.Create() does not allocate memory, or because it creates an instance of the base class type, rather than the given class type. As a result, I think create needs to be *in* the try
Graza
I'm using D2006 too and happily use Create on class type references without NewInstance. And frankly, I would be surprised if this was any different in, say, D5.
Ulrich Gerhardt
Yup, there is no difference between classname.create and classtypevar.create. Both create an instance and call the constructor.
Gamecat
Graza, you are wrong. Gamecat's solution works in every version of Delphi I can remember. Back before D4, anyway. You are also wrong about putting Create in the try. See Allen Bauer's blog for why.
Craig Stuntz
If Create() is virtual, and AClass refers to a class that is a *descendant* of TMyClass (eg TMySubClass = class(TMyClass), and then CreateExecute is called with TMySubClass as the argument), will calling AClass.Create() create a TMyClass object, or a TMySubClass object?
Graza
Actually, I just tested and it appears that the correct class is indeed created. I wonder why they do it this way in CreateForm()?
Graza
Graza, yes, *if* the constructor is virtual. (As with Gamecat's example.)
Craig Stuntz
Constructor is virtual. I've now gone from the test apps used to check doing it *without* NewInstance, into production code, and all is working. The ways suggested here were always how I'd instinctually have done it, but something in past projects led me to the NewInstance route for some reason.
Graza
NewInstance is used in VCL's TApplication to create Forms... Check .dpr file ;)
DiGi
I'm pretty sure I used CreateForm() as an example some time in the past because I couldn't get the more "straightforward" code working for some reason. I'm a little baffled at this point though, which CreateForm() uses NewInstance (or at least why it still uses it)
Graza
+1  A: 

Edit:

Didn't fully remember how it was in old delphi versions but apparently this should work in all based on other replies.

Note, Create has been calling Destroy on fail for as long as I can remember. It shouldn't be after I think.

Code would be:

procedure TPageClassFactory.TryExecute(ScrnClass: TCustomPageClass);
var
  ScrnObj: TCustomPage;
begin
  ScrnObj := ScrnClass.Create(Self);  // Exception here calls the destructor
  try
    ScrnObj.Execute; // Exception here means you need to free manually      
  finally
    FreeAndNil(ScrnObj); // Be free!
  end;
end;

I removed the result returned by the original function as it can never be false, only "unassigned" (exception) or true. You could after all get an exception before you assign result to false. ;)

PetriW
Graza
The point is that (at least under Delphi 2007) the raised exception in the constructor leads to the destructor being called "behind-the scene", so doing it again in the finally block is an error.
mghie
I've only messed around with Create in this manner in later versions and you are indeed correct in that the destructor is called if Create fails. How it is for very early delphis I don't know, the poster did not specify a version.
PetriW
Ah, I actually had some code above the creation in my original code, which *can* result in FALSE, but didn't think to remove the result from the "cut-down" version I posted
Graza
Just tested and indeed the destructor is called even if Create is called in this way. Which I think is a bit strange... an object can call its own create method after construction, which is *not* reconstructing it, so why is Destroy called? No problem though - you've given me a better way to do this
Graza
Destructors have *always* been called upon construction failure. And NewInstance has *never* been required in place of a bona-fide constructor call, for all versions of Delphi, even the first one. If your experience is otherwise, then you were doing something wrong.
Rob Kennedy
+1  A: 

Re your question about memory being leaked when Create() raises an exception: You should try it out for yourself. I just did on Delphi 2007, and with your code FastMM4 shows an error dialog about the attempt to call a virtual method on an already freed object, namely Destroy(). So the exception in Create will already lead to the destructor being called and the memory being freed, so your code is actually wrong. Stick to the idiom used in the answer by Gamecat, and everything should work.

Edit:

I just tried on Delphi 4, and the behaviour is the same. Test code:

type
  TCrashComp = class(TComponent)
  public
    constructor Create(AOwner: TComponent); override;
    destructor Destroy; override;
  end;

constructor TCrashComp.Create(AOwner: TComponent);
begin
  inherited Create(AOwner);
  raise Exception.Create('foo');
end;

destructor TCrashComp.Destroy;
begin
  Beep;
  inherited Destroy;
end;

procedure TForm1.Button1Click(Sender: TObject);
var
  C: TComponent;
begin
  C := TComponent(TCrashComp.NewInstance);
  try
    C.Create(nil);
    C.Tag := 42;
  finally
    C.Free;
  end;
end;

With FastMM4 the Free in the finally block gives the same error, because C has been freed already. On application shutdown the exception and the exception string are reported as memory leaks, though. This is however not a problem with the code, but with the runtime.

mghie
Using BDS2006 - does this apply (destroy called automatically) when Create() is being called in the way I am calling it (in this case it is not used as a "constructor" and does not allocate memory for the object, but is simply a method which initialises things).(AObject.Create <> TObject.Create)
Graza
If Create() overrides the method in the base class it will work. If not you should get a warning about hiding a method, and it probably will not work. Not that you would do that...
mghie
Actually, I don't understand the need for NewInstance, at all. I have never used it in similar places.
mghie
NewInstance is used in a lot of examples, maybe it's a remnant from an early Delphi or C++ Builder and everyone since has just copied. ;)
PetriW
This is kind of weird, as even the Delphi 1 help says not to call NewInstance as it will be called automatically by the constructor. I wonder what this is all about.
mghie
+3  A: 

There have been a few questions raised in comments that I'd like to clarify.

First is the continued myth that the constructor needs to be virtual. It does not. Consider this example:

type
  TBase = class
    constructor Create(x: Integer);
  end;
  TDerived = class(TBase)
    field: string;
  end;
  TMetaclass = class of TBase;

var
  instance: TBase;
  desiredClass: TMetaclass;
begin
  desiredClass := TDerived;
  instance := desiredClass.Create(23);
  Assert(instance.ClassName = 'TDerived');
  Assert(instance is TDerived);
  Assert(instance.field = '');
end;

The created object will be a full-fledged instance of class TDerived. Enough memory will have been allocated to hold the string field, which didn't exist in the base class.

There are two conditions that must be true before you'll need a virtual constructor:

  1. The constructor will be called virtually. That is, you'll have a variable of the base-class metaclass type, and it will hold a value of a derived class, and you will call a constructor on that variable. That's demonstrated in the code above. If all your constructor calls are directly on the class names themselves (i.e., TDerived.Create(23)), then there's nothing to be gained from virtual methods.
  2. A subclass of the base class will need to override the constructor to provide class-specific initialization. If all descendants use the same construction, and only vary in other methods, ten there's no need to make the constructor virtual.

What's important to realize here is that those two rules are no different from the factors that determine when the make any other method virtual. Constructors aren't special in that regard.

The constructor knows which class to construct based not on the class where the constructor was defined, but on the class the constructor was called on, and that class is always passed as a hidden first parameter for every constructor call.


Second is the issue of whether NewInstance should be called in place of or in addition to the constructor. I think other comments have already established that it has nothing to do with compatibility with older Delphi versions. All versions have supported calling constructors on class references without the need for NewInstace. Rather, the confusion comes from looking at TApplication.CreateForm and treating it as an example of how things should be done. That's a mistake.

CreateForm calls NewInstance before calling the constructor because CreateForm's primary reason for existence is to ensure that the global form variable that the IDE declares is valid during the form's own event handlers, including OnCreate, which runs as part of the constructor. If the CreateForm method had done the usual construction pattern, then the global form variable would not yet have had a valid value. Here's what you might have expected to see:

TComponent(Reference) := InstanceClass.Create(Application);

Simple and obvious, but that won't work. Reference won't get assigned a value until after the constructor returns, which is long after the form has triggered some events. If you follow good programming practice and never refer to that variable from within the form class itself, then you'll never notice. But if you follow the documentation's instructions, which are written for an inexperienced audience, then you will refer to the global form variable from within the form's own methods, so the CreateForm method does what it can to make sure it's assigned in time.

To do that, it uses a two-step construction technique. First, allocate memory and assign the reference to the global variable:

Instance := TComponent(InstanceClass.NewInstance);
TComponent(Reference) := Instance;

Next, call the constructor on the instance, passing the TApplication object as the owner:

Instance.Create(Self);

It's my opinion that CreateForm should be called exactly once in any program. I'd prefer zero times, but it has the side effect of defining Application.MainForm, which is important for other aspects of a Delphi program.


Third is the notion that it's unusual for an object to call a constructor on itself.

In fact, this happens all the time. Every time you call an inherited constructor, you're calling a constructor on an object that already exists. The inherited constructor is not allocating a new object. Likewise, the VCL has some examples of non-inherited calls of constructors. TCustomForm.Create delegates much of its construction tasks to its CreateNew constructor.

Rob Kennedy
Thanks for the extra info! :) Good read.
PetriW
Brilliant!Re CreateForm(): That makes perfect sense. Really, it sounds like a workaround due to the fact that the default project templates encourage the use of Global variables (which I've always thought was naughty of Borland!)
Graza
Re #3: Probably its best that some sort of "InitMembers" method is declared instead, but if for example an object calls its own Create method long after it has been created, in order to reinitialise itself, would it be freed at that point if an exception occurred?
Graza
No, Graza, the destructor won't get called in that case. Constructors know whether they're supposed to do allocation before they initialize. If they're just initializing, they don't set up a stack frame to make the destructor get called.
Rob Kennedy
Good to know. Then it still seems a bit odd that in the case of Application.CreateForm() that the destructor gets called on exception, as in this case Create() is not allocating memory, InitInstance() is. But I'm now steering clear of that way of doing things, so I'll happily leave that unexplained
Graza