views:

863

answers:

2

I apologize in advance for a newbie question, but why do I get "Access violation" error with the code below (on the "Create(SelectorForm);" line)? I tried using the main form as the owner, but it didn't make any difference.

var
  SelectorForm: TSelectorForm;
  ArrayOfImages: Array [1..10] of TImage;

implementation

procedure TSelectorForm.FormCreate(Sender: TObject);
var
  Loop: Byte;
begin
  for Loop := 1 to 10 do
  begin
    with ArrayOfImages[Loop] do
    begin
      Create(SelectorForm);
    end;
  end;
end;
+14  A: 

The problem is that you are effectively doing this:

var
  imageVariable: TImage;
begin
  imageVariable.Create (ParentForm);
end;

Which is wrong because "Create" method is being called on the variable which hasn't been assigned yet.

You should do this:

var
  imageVariable: TImage;
begin
  imageVariable := TImage.Create (ParentForm);
  try
    //use the object
  finally
    FreeAndNil (imageVariable);
  end;
end;

Or more specifically in your code:

for Loop := 1 to 10 do
begin
  ArrayOfImages[Loop] := TImage.Create (Self);
end;

Don't forget to free the objects

EDIT: Accepting @andiw's comment and taking back the tip of freeing objects. EDIT2: Accepting @Gerry's comment and using Self as owner.

Hemant
In my understanding the TImages will be freed automatically when SelectorForm is destroyed since it's been passed as their owner - or am I getting something wrong?
Andreas Wieland
@andiw: You are right. There is no need to free the objects if you assign the form as their owner.
Hemant
Wouldn't TImage.Create(Self); be better. It's nearly always a bad idea to use a specific form instance variable within a method of the form's class - it will fail if someone creates an instance with another name.
Gerry
A: 

There are a lot of problems with the above code. (don't use the "With" like that for a start, don't use a Byte for your loop var)

My assumption is that you ultimately want an array of instances of TImage's created with a form as the parent.

so based on that assumtion...you want something like (untested)

var
  ArrayOfImages: Array [0..9] of TImage;  
  i : integer;
begin
  for i := 0 to 9 do
  begin
    ArrayOfImages[i] := TImage.Create(theForm);
  end;

end;

Now note, you will be responsible for cleaning up the array when you are finished using it, you will need to call free on each of the Image instances.

Tim Jarvis
Using byte for the loop variable certainly doesn't qualify as a problem.
mghie
nope, just a style thing. Nothing wrong with limiting the range for the loop, but byte is typically a meaningful size, I would be spending a couple extra secs looking at this and wondering why the byte...
Tim Jarvis
No, he doesn't need to clean the instances. theForm will do it. And the array is a global. It'll die when the unit is finalized.
Fabricio Araujo