Is it memory safe to provide an object as a function result?

Question

Here I provide simple piece of code.

function GetStringList:TStringList;
var i:integer;
begin
   Result:=TStringList.Create;
   Result.Add('Adam');
   Result.Add('Eva');
   Result.Add('Kain');
   Result.Add('Abel');
end;


procedure ProvideStringList(SL:TStringList);
var i:integer;
    Names:TStringList;
begin
   Names:=TStringList.Create;
   Names.Add('Adam');
   Names.Add('Eva');
   Names.Add('Kain');
   Names.Add('Abel');
   SL.Assign(Names);
   Names.Free;
end;


procedure TForm1.btn1Click(Sender: TObject);
var SL:TStringList;
    i:integer;
begin
   SL:=TStringList.Create;
   SL.Assign(GetStringList);
   for i:=0 to 3 do ShowMessage(SL[i]);
   SL.Free;
end;


procedure TForm1.btn2Click(Sender: TObject);
var SL:TStringList;
    i:integer;
begin
   SL:=TStringList.Create;
   ProvideStringList(SL);
   for i:=0 to 3 do ShowMessage(SL[i]);
   SL.Free;
end;

And now the question: what will happen to result object in function GetStringList:Tstringlist, which is created, but never freed? (I call 2 times Create and only 1 time Free) Is it memory safe to provide objects by function or should I use procedures to do this task, where object creation and destroying is simply handled (procedure ProvideStringlist)? I call 2 times Create and 2 times Free. Or is there another solution?

Thanx in advance

Lyborko

Answer 1

I don't know what you mean by safe, but it is common practice. The caller of the function becomes responsible for freeing the returned object:

var
   s : TStringList; 
begin
   s := GetStringList;
   // stuff
   s.free;
end;

Answer 2

It is the usage that is the leak, not the construct itself.

var sl2 : TStringlist;

sl2:=GetStringList;
sl.assign(sl2);
sl2.free;

is perfectly fine, or easier even,

sl:=getstringlist;
// no assign, thus no copy, one created one freed.
sl.free;

Answer 3

In btn1Click you should do:

var sl2: TStringList;

sl2 := GetStringList:
SL.Assign(sl2);
sl2.Free;

In btn2Click you don't have to create an instance of SL before calling ProvideStringList to not create a memory leak.

Answer 4

No, it is not "memory safe". When you create an object, someone has to free it.

Your first example leaks memory:

SL:=TStringList.Create;
SL.Assign(GetStringList);   // <-- The return value of GetStringList is 
                            //     used, but not freed.
for i:=0 to 3 do ShowMessage(SL[i]);
SL.Free;

The second example works fine, but you don't have to create and free an additional temporary instance (Names)

In general, the second example is slightly better, because it is obvious, who is responsible for the creation and destruction of the list. (The caller) In other situations, a returned object must be freed by the caller or perhaps it's forbidden. You can't tell from the code. If you must do so, it's good practice to name your methods accordingly. (CreateList is better than GetList).

Answer 5

Is it memory safe to provide an object as a function result?

It is possible, but it needs attention from the implementor and the call.

1. Make it clear for the caller, the he controls the lifetime of the returned object
2. Make shure you don't have a memory leak when the function fails.

For example:

function CreateBibleNames: TStrings;
begin
  Result := TStringList.Create;
  try
    Result.Add('Adam');
    Result.Add('Eva');
    Result.Add('Kain');
    Result.Add('Abel');
  except
    Result.Free;
    raise;
  end;      
end;

But in Delphi the most commen pattern for this is:

procedure GetBibleNames(Names: TStrings);
begin
  Names.BeginUpdate;
  try
    //perhaps a Names.Clear here
    //but I don't use it often because the other
    //way is more flexible for the caller

    Names.Add('Adam');
    Names.Add('Eva');
    Names.Add('Kain');
    Names.Add('Abel');
  finally
    Names.EndUpdate;
  end;      
end;

so the caller code can look like this:

procedure TForm1.btn1Click(Sender: TObject);
var
  Names: TStrings;
  i:integer;
begin
  Names := CreateBibleNames;
  try
    for i := 0 to Names.Count -1 do 
      ShowMessage(Names[i]);
  finally 
    Names.Free;
  end;
end;

and the other, more common version:

procedure TForm1.btn1Click(Sender: TObject);
var
  Names: TStrings;
  i:integer;
begin
  Names := TStringList.Create;
  try
    GetBibleNames(Names);
    for i := 0 to Names.Count -1 do 
      ShowMessage(Names[i]);
  finally 
    Names.Free;
  end;
end;

(I have no compiler at the moment, so perhaps there are some errors)

Answer 6

I use a combination of both idioms. Pass the object as an optional parameter and if not passed, create the object. And in either case return the object as the function result.

This technique has (1) the flexibility of the creation of the object inside of the called function, and (2) the caller control of the caller passing the object as a parameter. Control in two meanings: control in the real type of the object being used, and control about the moment when to free the object.

This simple piece of code exemplifies this idiom.

function MakeList(aList:TStrings = nil):TStrings;
 var s:TStrings;
 begin
   s:=aList;
   if s=nil then 
     s:=TSTringList.Create;
   s.Add('Adam');
   s.Add('Eva');
   result:=s;
 end;

And here are three different ways to use it

simplest usage, for quick and dirty code

var sl1,sl2,sl3:TStrings;
sl1:=MakeList;

when programmer wants to make more explicit ownership and/or use a custom type

sl2:=MakeList(TMyStringsList.create);

when the object is previously created

sl3:=TMyStringList.Create;
....
MakeList(sl3);

Answer 7

Memory safety is a stricter variant of type safety. For memory safety, you typically need a precise garbage collector and a type system which prevents certain kinds of typecasts and pointer arithmetic. By this metric, Delphi is not memory safe, whether you write functions returning objects or not.

Answer 8

These are the very kinds of questions I grappled with in my early days of Delphi. I suggest you take your time with it:

• write test code with debug output
• trace your code step-by-step
• try different options and code constructs
• and make sure you understand the nuances properly;

The effort will prove a great help in writing robust code.

Some comments on your sample code...

1. You should get into the habit of always using resource protection in your code, even in simple examples; and especially since your question pertains to memory (resource) protection.
2. If you name a function GetXXX, then there's no reason for anyone to suspect that it's going to create something, and they're unlikely to protect the resource. So careful naming of methods is extremely important.
3. Whenever you call a method that creates something, assume it's your responsibility to destroy it.
4. I noticed some code that would produce Hints from the compiler. I recommend you always eliminate ALL Hints & Warnings in your programs.
   • At best a Hint just means some arbitrary redundant code (excesses of which make maintenance more difficult). More likely it implies you haven't finished something, or rushed it and haven't finished testing/checking.
   • A Warning should always be taken seriously. Even though sometimes the compiler's concern is a logical impossibility in the specific situation, the warning may indicate some subtle language nuance that you're not aware of. The code can always be rewritten in a more robust fashion.
   • I have seen many examples of poor resource protection where there is a compiler warning giving a clue as to the problem. So check them out, it will aid in the learning.

If an exception is raised in a method that returns a new object, care should be taken to ensure there isn't a memory leak as a result.

//function GetStringList:TStringList;
function CreateStringList:TStringList; //Rename method lest it be misinterpreted.
//var i: Integer; You don't use i, so why declare it? Get rid of it and eliminate your Hints and Warnings!
begin
  Result := TStringList.Create;
  try //Protect the memory until this method is done; as it can **only** be done by **this** method!
    Result.Add('Adam');
    Result.Add('Eva');
    Result.Add('Kain');
    Result.Add('Abel');
  except
    Result.Destroy;  //Note Destroy is fine because you would not get here if the line: Result := TStringList.Create; failed.
    raise; //Very important to re-raise the exception, otherwise caller thinks the method was successful.
  end;
end;

A better name for the following would be PopulateStringList or LoadStringList. Again, resource protection is required, but there is a simpler option as well.

procedure ProvideStringList(SL:TStringList);
var //i:integer;  You don't use i, so why declare it? Get rid of it and eliminate your Hints and Warnings!
    Names:TStringList;
begin
  Names:=TStringList.Create;
  try //ALWAYS protect local resources!
    Names.Add('Adam');
    Names.Add('Eva');
    Names.Add('Kain');
    Names.Add('Abel');
    SL.Assign(Names);
  finally //Finally is the correct choice here
    Names.Free; //Destroy would also be okay.
  end;
end;

However; in the above code, creating a temporary stringlist is overkill when you could just add the strings directly to the input object.
Depending on how the input stringlist is used, it is usually advisable to enclose a BeginUpdate/EndUpdate so that the changes can be handled as a batch (for performance reasons). If your method is general purpose, then you have no idea of the origin of the input, so you should definitely take the precaution.

procedure PopulateStringList(SL:TStringList);
begin
  SL.BeginUpdate;
  try //YES BeginUpdate must be protected like a resource
    SL.Add('Adam');
    SL.Add('Eva');
    SL.Add('Kain');
    SL.Add('Abel');
  finally
    SL.EndUpdate;
  end;
end;

our original code below had a memory leak because it called a method to create an object, but did not destroy. However, because the method that created the object was called GetStringList, the error is not immediately obvious.

procedure TForm1.btn1Click(Sender: TObject);
var SL:TStringList;
    i:integer;
begin
  //SL:=TStringList.Create; This is wrong, your GetStringList method creates the object for you.
  //SL.Assign(GetStringList);
  SL := CreateStringList;  //I also used the improved name here.
  try //Don't forget resource protection.
    for i:=0 to 3 do ShowMessage(SL[i]);
  finally
    SL.Free;
  end;
end;

The only error in your final snippet was the lack of resource protection. The technique used is quite acceptable, but may not be ideally suited to all problems; so it helps to also be familiar with the previous technique.

procedure TForm1.btn2Click(Sender: TObject);
var SL:TStringList;
    i:integer;
begin
  SL:=TStringList.Create;
  try //Be like a nun (Get in the habit)
    ProvideStringList(SL);
    for i:=0 to 3 do ShowMessage(SL[i]);
  finally
    SL.Free;
  end;
end;