I then decided to try out TMyRect in a form.When doing an assignment of class instances you only copy a reference to the class instance. The form (or more correctly the canvas) still has that reference and as the canvas was the one that created the pen and brush it's also going to free it. So when you free it as well in the destructor of TMyRect you essentially free it twice (it doesn't matter if you or the canvas frees the two first, because one of the two will free an already freed object).
MyRect.Brush:=Form1.Canvas.Brush; MyRect.Pen:=Form1.Canvas.Pen;
By chance I included in Form1.Close the following:You release your reference to the class instance. And calling Free on a Nil class reference is a no-op.
MyRect.Brush:=nil; MyRect.Pen:=nil;
That prevented the error on Close, so I moved that code to the Destructor:
destructor TMyRect.Free; begin Brush:=nil; Pen:=nil; Brush.Free; Pen.Free; end;
... and that worked.
@Thaddy: how about explaining Bazzao the real problem they have instead of some completely unrelated stuff?Well, you just explained it is not unrelated stuff 8-) You are much better in explaining, but at least I gave it a try... :'(
if Assigned(oMyObject) then FreeAndNil(oMyObject);
Yes, it is unrelated, cause as you might notice I didn't mention FreeAndNil at all. In fact more often than not that procedure is simply overused. One needs to learn when it is useful to use.Then again, I explained what the issue was.... It is not often I disagree with you. But if you say so..., OK?
Sorry but that is redundant. Both Free and FreeAndNIl already test first whether the the object is assigned. That's why they are "safe".
One may want to do it to avoid the subsequent call but nothing else.
Uh, why has no one pointed out how wrong their destructor implementation is?Did you read their whole message? They wrote that setting Brush and Pen to Nil solves their problem which is because those two were copied references from the form's canvas. So they fixed a symptom and I tried to explain the real cause of their problem.
destructor TMyRect.Free; begin Brush:=nil; Pen:=nil; Brush.Free; Pen.Free; end;
Do not ever call your destructor "Free". Free is an existing procedure, that itself will properly call any available destructor for the class.
Also, setting any class to "nil" and then calling "Free" means the call to "Free" is useless, because the implementation of "Free" looks like this:
1. form1 is owner of pen, and destroys it.That's what I wrote in my post as well.
2. you assigned pen to tmyrect and free it.
-> it gets destroyed twice -> exception
now you NIL it so the tmyrect.free doesn't do anything, but tmyrect simply shouldn't free things that the form still owns..
@Thaddy: how about explaining Bazzao the real problem they have instead of some completely unrelated stuff?
When doing an assignment of class instances you only copy a reference to the class instance. The form (or more correctly the canvas) still has that reference and as the canvas was the one that created the pen and brush it's also going to free it. So when you free it as well in the destructor of TMyRect you essentially free it twice (it doesn't matter if you or the canvas frees the two first, because one of the two will free an already freed object).
You release your reference to the class instance. And calling Free on a Nil class reference is a no-op.
As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):
constructor TMyRect.Create; begin Brush := TBrush.Create(Nil); Pen := TPen.Create(Nil); end;
And where you currently assign the references:
MyRect.Brush.Assign(Form1.Canvas.Brush); MyRect.Pen.Assign(Form1.Canvas.Pen);
Alternatively you accept that your TMyRect merely has copies of the references and don't free them. You could also use a property setter and check whether the owner of the Brush and Pen instances is already set to something and then set a Boolean (for each of the two) that does stop the destructor from calling Free in that case.
In any case you don't need the assignments to Nil anymore.
As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):
Error: Wrong number of parameters specified for call to "Create"
I didn't remember whether these types have an Owner or not. Apparently they don't :-[
As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):
constructor TMyRect.Create; begin Brush := TBrush.Create(Nil); Pen := TPen.Create(Nil); end;
Nope. (Nil) not liked.QuoteError: Wrong number of parameters specified for call to "Create"