Why this code fail?

[Magno 4]

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.

 

[Lars Fosdal 1792]

value of myQuery is undefined at call to createMyQuery.

Check your hints and warnings.

[Gustav Schubert 25]

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 April 23, 2020 by Gustav Schubert
var is not needed - but wait - it is !

[Anders Melander 1782]

 

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.

[aehimself 396]

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.

[Attila Kovacs 629]

@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 April 24, 2020 by Attila Kovacs

[aehimself 396]

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.

[Attila Kovacs 629]

@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?

[aehimself 396]

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 April 24, 2020 by aehimself

[Attila Kovacs 629]

@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 April 24, 2020 by Attila Kovacs

[aehimself 396]

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.

[Magno 4]

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;

 

 

[Magno 4]

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.

[aehimself 396]

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.

[Gustav Schubert 25]

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.

[Remy Lebeau 1394]

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.