Recent

Author Topic: A function returning a TStringlist: is this advisable?  (Read 11176 times)

JD

  • Hero Member
  • *****
  • Posts: 1848
Re: A function returning a TStringlist: is this advisable?
« Reply #15 on: January 20, 2017, 09:33:39 pm »
Interesting discussion. I learned some of these programming oops the hard way. I was seduced by the facility of StringList Value-Key pairs and used it everywhere in an application.

However it became a nightmare to maintain after several years. Now I'm doing extensive code rewrite and replacing the StringLists with Records. Nothing to create or free. Whew!
Windows - Lazarus 2.1/FPC 3.2 (built using fpcupdeluxe),
Linux Mint - Lazarus 2.1/FPC 3.2 (built using fpcupdeluxe)

mORMot; Zeos 8; SQLite, PostgreSQL & MariaDB; VirtualTreeView

cov

  • Full Member
  • ***
  • Posts: 241
Re: A function returning a TStringlist: is this advisable?
« Reply #16 on: January 20, 2017, 10:13:31 pm »
Thank you all.

Some serious issues to consider, but I hope we don't come to blows over it!

jcmontherock

  • Full Member
  • ***
  • Posts: 234
Re: A function returning a TStringlist: is this advisable?
« Reply #17 on: April 24, 2018, 05:05:09 pm »
I suggest another approach which works with recursive functions/procedures:
Code: Pascal  [Select][+][-]
  1. procedure CallingProc;
  2. var
  3.   strList: TStringList;
  4. begin
  5. strList := TStringList.Create;
  6. CalledProc(strList);
  7. ....
  8. strList.Free;
  9. end;
  10.  
  11. Procedure CalledProc(strList: TStringList);
  12. begin
  13.   strList.Add('A string');
  14.   if cond = OK then CalledProc(strList);
  15.   ...
  16. end;
  17.  
Windows 11 UTF8-64 - Lazarus 3.2-64 - FPC 3.2.2

del

  • Sr. Member
  • ****
  • Posts: 258
Re: A function returning a TStringlist: is this advisable?
« Reply #18 on: October 04, 2019, 07:22:30 pm »
OK - I was just gonna do something like this and google led me here. I was going to have a function generate an object of a class that basically calculates and manages a set of kernel indices. In C++ this is not a problem cuz IIRC during the return phase of the function the object's copy constructor is invoked and the original instance that was actually generated inside the function goes out of scope and its destructor basically ends its existence and takes it safely out of play.

I'm getting the impression from this thread that in Pascal this would be a reckless practice. Am I getting this right? I understand "var" and passing by reference, or making this creation / initialization function one of the class's constructors. So there are workarounds. I just want to know definitively for future reference.

Thaddy

  • Hero Member
  • *****
  • Posts: 14201
  • Probably until I exterminate Putin.
Re: A function returning a TStringlist: is this advisable?
« Reply #19 on: October 04, 2019, 08:20:05 pm »
Code: Pascal  [Select][+][-]
  1. Procedure CalledProc(const strList: TStringList);
Specialize a type, not a var.

lucamar

  • Hero Member
  • *****
  • Posts: 4219
Re: A function returning a TStringlist: is this advisable?
« Reply #20 on: October 04, 2019, 08:28:18 pm »
It's quite an old bone of contention but, IMHO, there's nothing intrinsically wrong with returning an object created inside a function and, in some ocassions, it can be quite usefull.

The problem arises because it's easy to "forget" to free/release the returned object, so the usual way to implement such functions is to make them accept a parameter of the desired class and make the funciton create/use it. The election is yours.

A good example of this are the overloaded FindAllFiles(): one requires you to pass a TStringList created outside where it returns the list of files, while the other creates  internally (and returns) a TStringList which you can assign to an object reference. That is, they can be used as:

Code: Pascal  [Select][+][-]
  1. { As a parameter ... }
  2. AList := TStringList.Create;
  3. try
  4.   FindAllFiles(AList, ...);
  5.   {Do something with AList}
  6. finally
  7.   AList.Free;
  8. end;
  9.  
  10. { As the function result }
  11. try
  12.   AList := FindAllFiles(...);
  13.   {Do something with AList}
  14. finally
  15.   AList.Free;
  16. end;

This last form allows you to do, for example:
Code: Pascal  [Select][+][-]
  1. with FindAllFiles(...) do
  2. try
  3.   {Do something with the result}
  4. finally
  5.   Free;
  6. end;

As you see, the key is that the object returned by the function should be freed inside the callee, so you have to consider (and not forget!) the call to FindAllFiles as of it were an "special" contructor. Provided you (and every other user of your code) remember that, there should be no problem, although it's considered good practice (in that case) to provide an overload requiring a parameter of the proper class and using it unless otherwise needed.
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus/FPC 2.0.8/3.0.4 & 2.0.12/3.2.0 - 32/64 bits on:
(K|L|X)Ubuntu 12..18, Windows XP, 7, 10 and various DOSes.

wp

  • Hero Member
  • *****
  • Posts: 11854
Re: A function returning a TStringlist: is this advisable?
« Reply #21 on: October 04, 2019, 09:56:30 pm »
A good example of this are the overloaded FindAllFiles(): one requires you to pass a TStringList created outside where it returns the list of files, while the other creates  internally (and returns) a TStringList which you can assign to an object reference.
An example for a hard-to-find memory leak caused by using FindAllFiles in the way described in the last sentence:
Code: Pascal  [Select][+][-]
  1.   Listbox1.Items := FindAllFiles(...);
The assignment operator := means here that the StringList created by the FindAllFiles funtion is copied, item by item, to the Items administrated by the Listbox. While the copy in Items is destroyed by the stringlist it's original is kept as a memory leak because the result variable of the FindAllFiles function is not available any more for calling .Free.

del

  • Sr. Member
  • ****
  • Posts: 258
Re: A function returning a TStringlist: is this advisable?
« Reply #22 on: October 04, 2019, 10:27:38 pm »
An example for a hard-to-find memory leak caused by using FindAllFiles in the way described in the last sentence:
Code: Pascal  [Select][+][-]
  1.   Listbox1.Items := FindAllFiles(...);
The assignment operator := means here that the StringList created by the FindAllFiles funtion is copied, item by item, to the Items administrated by the Listbox. While the copy in Items is destroyed by the stringlist it's original is kept as a memory leak because the result variable of the FindAllFiles function is not available any more for calling .Free.
This is my understanding of the root of the problem. I can remember to Free the returned copy. You do that out of habit whenever you create an object using its normal constructors. But when the function returns a "deep copy" (if I'm using that term correctly), the original construction inside the function falls out of scope (so far so good) but the Pascal language does not have scope-based object destruction.

OTOH it's not a huge problem to create a true constructor whenever you need leak-free "function-like" behavior. For example - I use this array of kernel indices very often (some kernels have non-rectangular footprints) so I just make a "kernel" (as in filter) class and construct (create) whenever I need one. You end up with a variety of constructors - but I don't think that's abusing the concept.
 :D

MarkMLl

  • Hero Member
  • *****
  • Posts: 6676
Re: A function returning a TStringlist: is this advisable?
« Reply #23 on: October 04, 2019, 10:49:47 pm »
Quite frankly I think there are worse things to worry about, like the fact that a function can return (or populate) a stringlist without specifying that from that point onwards the content should be immutable.

One of my betes noires is this sort of thing:

with thisReturnsAnObject() do begin
...
  Free
end;

MarkMLl
MT+86 & Turbo Pascal v1 on CCP/M-86, multitasking with LAN & graphics in 128Kb.
Pet hate: people who boast about the size and sophistication of their computer.
GitHub repositories: https://github.com/MarkMLl?tab=repositories

lucamar

  • Hero Member
  • *****
  • Posts: 4219
Re: A function returning a TStringlist: is this advisable?
« Reply #24 on: October 04, 2019, 10:51:20 pm »
An example for a hard-to-find memory leak caused by using FindAllFiles in the way described in the last sentence:
Code: Pascal  [Select][+][-]
  1.   Listbox1.Items := FindAllFiles(...);
The assignment operator := means here that the StringList created by the FindAllFiles funtion is copied, item by item, to the Items administrated by the Listbox. While the copy in Items is destroyed by the stringlist it's original is kept as a memory leak because the result variable of the FindAllFiles function is not available any more for calling .Free.

Yeah, in cases like that one the other overload should be used, since the TStrings instance already exists:

Code: Pascal  [Select][+][-]
  1.   FindAllFiles(Listbox1.Items, ...);

Which emphasizes the fact that one *must* take extra care when using functions which create and return an object, to minimize the possibility of problems further on.

Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus/FPC 2.0.8/3.0.4 & 2.0.12/3.2.0 - 32/64 bits on:
(K|L|X)Ubuntu 12..18, Windows XP, 7, 10 and various DOSes.

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11383
  • FPC developer.
Re: A function returning a TStringlist: is this advisable?
« Reply #25 on: October 04, 2019, 11:47:27 pm »
Quite frankly I think there are worse things to worry about,

Yeah, I always mentally put them in the same category as freeandnil (like even on fields in destructors) or try-finally overuse(for code that can't throw exceptions other than stack or memory checks, in which case your program is usually dead anyway)

They are simply easy and common things to point out on forums, which is why it has become such enormous groupthink to advocate them.

winni

  • Hero Member
  • *****
  • Posts: 3197
Re: A function returning a TStringlist: is this advisable?
« Reply #26 on: October 05, 2019, 12:10:10 am »
BRAVO!

del

  • Sr. Member
  • ****
  • Posts: 258
Re: A function returning a TStringlist: is this advisable?
« Reply #27 on: October 05, 2019, 12:44:58 am »
To be candid - I'm not seeing this as a "problem". It's just a matter of awareness and clarity for somebody coming to Pascal from the C++ world. Associated with the absence of scope-based memory management is the memory leak you get when you instantiate an object inside a function and return its copy. You can free the copy, but you've lost contact with the still-alive original instantiation.

So if you like or need your code to use returned objects, then your returned objects must come from the object's constructor, so there is no memory leak and you can free it when desired. That's just the basic mechanics of the Pascal world. I'm not being judgemental.

FWIW I never use FreeAndNil. I don't want a blanket between me and what's going on.

del

  • Sr. Member
  • ****
  • Posts: 258
Re: A function returning a TStringlist: is this advisable?
« Reply #28 on: October 05, 2019, 02:19:13 am »
OK gang - what about this? Create a class that contains a TStringlist member and feed the constructor of your new class all the function parameters you need to build the desired TStringlist?

Your new class is just a vehicle to carry what you really want - the TStringlist - and you free the TStringlist as part of your destructor for the "vehicle" class. It's clean - you're not leaving any bits and pieces of stuff in memory. Whether or not you think this is an attractive approach - does it do the job of a leak-free function that returns a TStringlist?

MarkMLl

  • Hero Member
  • *****
  • Posts: 6676
Re: A function returning a TStringlist: is this advisable?
« Reply #29 on: October 05, 2019, 10:01:53 am »
OK gang - what about this? Create a class that contains a TStringlist member and feed the constructor of your new class all the function parameters you need to build the desired TStringlist?

That clashes with your "I don't want a blanket between me and what's going on." principle.

How about this: document your functions properly, and if in any doubt at all read your own documentation.

It is, after all, why programming languages have comments. And it is why the Lazarus IDE displays those comments (with an obvious caveat about getting the format right) when you hover your pointer over a function name.

(* If there are no serial ports on the system then return NIL, otherwise a
  TStringList.

 This returns an object, it is the caller's (eventual) responsibility to free
  this.
*)
FUNCTION EnumeratePorts: TStringList;

MT+86 & Turbo Pascal v1 on CCP/M-86, multitasking with LAN & graphics in 128Kb.
Pet hate: people who boast about the size and sophistication of their computer.
GitHub repositories: https://github.com/MarkMLl?tab=repositories

 

TinyPortal © 2005-2018