Reputation: 5050
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?
Upvotes: 3
Views: 1405
Reputation: 53366
You should put the create out of the try finally block.
But a better solution is:
type
TMyClass = class ()
public
constructor Create(...); virtual;
function 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;
Upvotes: 12
Reputation: 1245
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. ;)
Upvotes: 2
Reputation: 163247
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:
TDerived.Create(23)
), then there's nothing to be gained from virtual methods.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.
Upvotes: 4
Reputation: 32334
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.
Upvotes: 2