Jump to content

Recommended Posts

The code bellow uses FireDac to create a new query object. The if not Assigned() sometimes will bring garbage and make the routine fail because as it reads something, so the "query" won't be created and crashes.

 

procedure createMyQuery(var query: TFDQuery; Sql: String='');
begin
  if not Assigned(query) then // fail
     query := TFDQuery.Create(nil);
  query.Connection := myDatabase;
  query.SQL.Text := Sql;
end;

 

var

   myQuery: TFDQuery;

begin

    createMyQuery(myQuery,'select * from table');

   ...

end;

 

Anything I could make it ? The Delphi version is 10.2 update 3.

 

Share this post


Link to post

value of myQuery is undefined at call to createMyQuery.

Check your hints and warnings.

  • Like 2

Share this post


Link to post

MyQuery should be initialized to nil.

 

//procedure createMyQuery(query: TFDQuery; const Sql: String='');
procedure createMyQuery(var query: TFDQuery; const Sql: String='');
begin
  if query = nil then
    query := TFDQuery.Create(nil);
  query.Connection := myDatabase;
  query.SQL.Text := Sql;
end;

var
  myQuery: TFDQuery;

begin
  myQuery := nil;
  createMyQuery(myQuery,'select * from table');

end;

 

Edited by Gustav Schubert
var is not needed - but wait - it is !

Share this post


Link to post

 

21 minutes ago, Gustav Schubert said:

var is not needed

Yes it is. The code you have posted will leak the TFDQuery instance and not return anything.

  • Like 2

Share this post


Link to post
On 4/23/2020 at 3:02 PM, Anders Melander said:

Yes it is. The code you have posted will leak the TFDQuery instance and not return anything.

I thought class instances are always passed as references (pointers to the actual instance)...? I also do have a similar method to create and initialize the dataset and I think I didn't use var there. I do have to doublecheck it; I don't have access to that piece of code right now.

Share this post


Link to post

@aehimself "var" also enforces passing a variable to the method, in this case it would be bad if you could call it with nil.

 

Anyway, this kind of lazy initialization will fail on local variables as they are not automatically set to nil, I would forget it forever.

Edited by Attila Kovacs

Share this post


Link to post
Just now, Attila Kovacs said:

@aehimself "var" also enforces passing a variable to the method, in this case it would be bad if you could call it with nil.

You are absolutely right in this, I'm always declaring local variables though (and as it's my helper I'll not call it with nil) it seems to be irrelevant in my case. My real surprise is/was the leaking part. If my memory doesn't cheat and class instances are indeed passed as references to methods, there should not be any leaks in the above code, with var or not.

Share this post


Link to post

@aehimself If it's not marked as "var" the parameter is just a preinitialized local var which won't be freed, as this was a factory logic originally. Or am I missing something?

Share this post


Link to post
15 minutes ago, Attila Kovacs said:

@aehimself If it's not marked as "var" the parameter is just a preinitialized local var which won't be freed, as this was a factory logic originally. Or am I missing something?

If you mean you have to set your variable to nil before first calling this method there's no question about it. As for freeing, that should be handled in the same method, which calls this helper. Like...

Var
 myquery: TFDQuery
Begin
 myquery := nil;
 Try
  createMyQuery(myquery, 'SELECT * FROM USERS');
  [...]
 Finally
  myquery.Free;
 End;
End;

What I mean is, whether if var is declared in the definition of createMyQuery or not; it will not leak and will function correctly, as "myquery" will be passed as a reference (the object itself) and not a copy of it.

 

Edit: I'm an idiot. Instead of asking I could have made a test case to confirm. Will come back soon with my results.

 

Edit-edit: Disregard everything. Var is needed.

Procedure TForm1.SLCreateAdd(inSL: TStringList);
Begin
 If Not Assigned(inSL) Then inSL := TStringList.Create;
 inSL.Add('Hello');
 inSL.Add('World');
End;

procedure TForm1.Button1Click(Sender: TObject);
Var
 sl: TStringList;
begin
 sl := nil;
 Try
  SLCreateAdd(sl);
  ShowMessage(sl.Count.ToString);
 Finally
  sl.Free;
 End;
end;

Causes a nullreference exception and leaks a TStringList object.

Procedure TForm1.SLCreateAdd(Var inSL: TStringList);
Begin
 If Not Assigned(inSL) Then inSL := TStringList.Create;
 inSL.Add('Hello');
 inSL.Add('World');
End;

does not. I am obviously wrong with my memories and most probably used Var in my helper 🙂

 

 

Edited by aehimself

Share this post


Link to post

@aehimself Ah, I see. You are right, it should be ok. I have also used a helper like this for centuries, it was error prone and annoying initializing the querys to nil. Which is obviously the problem what OP has, unitialized parameter. Then I switched to ORM, since then I can't stand such code anymore 😉

Edited by Attila Kovacs

Share this post


Link to post
8 minutes ago, Attila Kovacs said:

it was error prone and annoying initializing the querys to nil.

And this is something I will never EVER argue about. For me it also makes the code harder to read, as I already got used to Create - Try - Finally - Free - End. I'll always be alarmed if this pattern is not followed. In my case unfortunately it's a bit different, as we have (I'd call them as) protocol descriptors in our custom dataset descendants, no direct SQL queries.

 

But I hate setting the variables to nil before.

  • Haha 1

Share this post


Link to post
On 4/23/2020 at 9:40 AM, Gustav Schubert said:

MyQuery should be initialized to nil.

 

I thought that Delphi could initialize it by default, like strings are null, integers are 0 and so on. Thank you


//procedure createMyQuery(query: TFDQuery; const Sql: String='');
procedure createMyQuery(var query: TFDQuery; const Sql: String='');
begin
  if query = nil then
    query := TFDQuery.Create(nil);
  query.Connection := myDatabase;
  query.SQL.Text := Sql;
end;

var
  myQuery: TFDQuery;

begin
  myQuery := nil;
  createMyQuery(myQuery,'select * from table');

end;

 

 

Share this post


Link to post
On 4/24/2020 at 7:03 PM, aehimself said:

And this is something I will never EVER argue about. For me it also makes the code harder to read, as I already got used to Create - Try - Finally - Free - End. I'll always be alarmed if this pattern is not followed. In my case unfortunately it's a bit different, as we have (I'd call them as) protocol descriptors in our custom dataset descendants, no direct SQL queries.

 

But I hate setting the variables to nil before.

Yep, but I think the helper with setting nil before is not so handy, but it is ok. This is the way, as I have spoken.

Share this post


Link to post
5 hours ago, Magno said:

Yep, but I think the helper with setting nil before is not so handy, but it is ok. This is the way, as I have spoken.

I'm not saying there's something wrong with this type of helpers (I have close to the same at work) it's just an "inconvenient" - or should I say unusual - format to follow. And - as @Attila Kovacs mentioned and your example shows - you have to be careful; as there's no way (according to my knowledge) in Delphi to see if a TObject is initialized or not.

  • Thanks 1

Share this post


Link to post
On 4/28/2020 at 3:37 PM, Magno said:

I thought that Delphi could initialize it by default, like strings are null, integers are 0 and so on.

If global variable myQuery was a field of a class then it would be automatically initialized to nil.
And if the procedure createMyQuery was a method of that class you would not need to pass the myQuery instance around as a parameter.
Classes can be used with console application as well, just saying.

Share this post


Link to post

Local variables that are un-managed types are not auto-initialized.  Object pointers are managed types only on ARC platforms (iOS and Android).  Strings are always managed types, integers are never managed types.  Only global variables and class members that are un-managed types get auto-initialized to zeros.

  • Like 2

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×