views:

260

answers:

6

I've just been debugging a problem with a function that returns a string that has got me worried. I've always assumed that the implicit Result variable for functions that return a string would be empty at the start of the function call, but the following (simplified) code produced an unexpected result:

function TMyObject.GenerateInfo: string;

        procedure AppendInfo(const AppendStr: string);
        begin
          if(Result > '') then
            Result := Result + #13;
          Result := Result + AppendStr;
        end;

begin
  if(ACondition) then
    AppendInfo('Some Text');
end;

Calling this function multiple times resulted in:

"Some Text"

the first time,

"Some Text"
"Some Text"

the second time,

"Some Text"
"Some Text"
"Some Text"

the third time, etc.

To fix it I had to initialise the Result:

begin
  Result := '';
  if(ACondition) then
    AppendInfo('Some Text');
end;

Is it necessary to initialise a string function result? Why (technically)? Why does the compiler not emit a warning "W1035 Return value of function 'xxx' might be undefined" for string functions? Do I need to go through all my code to make sure a value is set as it is not reliable to expect an empty string from a function if the result is not explicitly set?

I've tested this in a new test application and the result is the same.

procedure TForm1.Button1Click(Sender: TObject);
var
  i: integer;
  S: string;
begin
  for i := 1 to 5 do
    S := GenerateInfo;
  ShowMessage(S); // 5 lines!
end;
A: 

This looks like a bug in D2007. I just tested it in Delphi 2010 and got the expected behavior. (1 line instead of 5.)

Mason Wheeler
I just tested it in Delphi 2009, and got all five lines.
Andreas Rejbrand
It's a not bug - see my reply.
Alexander
But then is it a bug with D2010?
Sertac Akyuz
@Sertac Akyuz: I think that Mason Wheeler just didn't test is correctly. (I did the same mistake the first time I tested the code - didn't realise I had to call `GenerateInfo` several times.)
Andreas Rejbrand
Yes, I agree: I think that Mason Wheeler didn't test it properly. This issue exists in all Delphi versions (verified in D3-D2010).
Alexander
+3  A: 

We've run into this before, I think maybe as far back as Delphi 6 or 7. Yes, even though the compiler doesn't bother to give you a warning, you do need to initialize your string Result variables, for precisely the reason you ran into. The string variable is getting initialized -- it doesn't start as a garbage reference -- but it doesn't seem to get *re*initialized when you expect it to.

As for why it happens... not sure. It's a bug, so it doesn't necessarily need a reason. We only saw it happen when we called the function repeatedly in a loop; if we called it outside a loop, it worked as expected. It looked like the caller was allocating space for the Result variable (and reusing it when it called the same function repeatedly, thus causing the bug), rather than the function allocating its own string (and allocating a new one on each call).

If you were using short strings, then the caller does allocate the buffer -- that's long-standing behavior for large value types. But that doesn't make sense for AnsiString. Maybe the compiler team just forgot to change the semantics when they first implemented long strings in Delphi 2.

Joe White
The caller do not allocate space but passes a valid reference to an initialized variable, at least the general conception is that as I understand it. It's got reports: http://qc.embarcadero.com/wc/qcmain.aspx?d=894, http://qc.embarcadero.com/wc/qcmain.aspx?d=32556 probably some others as well.
Sertac Akyuz
It's a not bug - see my reply.
Alexander
+14  A: 

This is not a bug, but "feature":

For a string, dynamic array, method pointer, or variant result, the effects are the same as if the function result were declared as an additional var parameter following the declared parameters. In other words, the caller passes an additional 32-bit pointer that points to a variable in which to return the function result.

I.e. your

function TMyObject.GenerateInfo: string;

Is really this:

procedure TMyObject.GenerateInfo(var Result: string);

Note "var" prefix (not "out" as you may expect!).

This is SUCH un-intuitive, so it leads to all kind of problems in the code. Code in question - just one example of results of this feature.

See and vote for this request.

Alexander
You can't in good faith call this a feature. Misfeature, perhaps: http://catb.org/jargon/html/M/misfeature.html. But a definite +1 for the link to the docs.
Joe White
I do not think that any of the consequences are unintuitive. I have always considered `result` as a local variable, and these generally (integers, reals, booleans, ...) need to be initialized manually. So I react rather strongly to the OP's code that is lacking the `result := ''` first line.
Andreas Rejbrand
*> I have always considered result as a local variable* Which is **not**. Result is output argument of function. It's not local variable.
Alexander
@Alexander: I know.
Andreas Rejbrand
@Alexander: I wonder why someone gave you a downvote... Your answer is definitely better than mine at least...
Andreas Rejbrand
@Andreas Rejbrand Well, may be there are people, who actually exploit this "feature" as feature, and they dpon't want it to be "fixed".
Alexander
Maybe this was more intuitive in the old days when we had no "Result variable", but had to assign the result value to the function name instead (i.e. GenerateInfo := 'Some Text' instead of Result := 'Some Text'). That way you could look at a function as "an advanced global variable with an associated code block", and you had to explicitly define the local variables you needed in the function. If you try to use the code GenerateInfo := GenerateInfo + 'Some Text', it just makes no sense (it actually creates an infinite recursive loop). +1 to Andreas' point: Write clean code that makes sense.
Jørn E. Angeltveit
@Alexander: Even the best can misunderstand the concept of the "Result-variable". See Zarko Gajic at http://delphi.about.com/od/beginners/a/subroutines.htm: "Since every function implicitly has a local variable Result[...]"
Jørn E. Angeltveit
@Jørn E. Angeltveit Okay, so you (and Andreas) are saying "init variables before use", right? No problem to me, makes sense. Now, try to apply this rule to function, which returns record with string. How we usually initialize record? Why, by using FillChar, of course! Can you guess what will FillChar do in this case? Since Result is actually an *input* variable - your "code written by good rules" will trash existing string pointer. Not pretty, right? So, it's not safe to think of Result as anything except the thing that it is: in/out argument.
Alexander
P.S. Note, that using FillChar on *real* local variable **will** work. That's because local variables aren't initialized. Result is not local variable and it, in fact, is **initialized** and may contain non-empty value. That is certanly unexpected. So, attempt to use FillChar fails.
Alexander
P.P.S. When speaking of "initialized" for local records, it may be a bit fuzzy. Local records aren't initialized in the sense that they aren't filled with zeros. Local records are "initialized" in the sense, that they contains zeroed fields of managed types (strings, interfaces, etc). Hope, you see, what I'm trying to say.
Alexander
@Alexander: Well, I'm not saying that you should initialization everything (in the sense of nil'ing every bit in the variable) before you use it in any way. I'm saying that you should write clean code that doesn't presume anything. If the function simply assigns a value to every field in the record, then that would be a good enough "initialization". So, how do I initialize a record? Create an init-function that initialized every field in the record with the preferred default values, and call this function instead of FillChar. In Delphi 2006 "record constructors" were introduced to do this.
Jørn E. Angeltveit
Couple of questions: is the exactly the same pointer always recycled to pass to the function from the calling proc's loop? If the compiler changed the param to "out" would all functions always emit a warning if the Result was uninitialised?
avenmore
First question: I think there is no guarantee for that.Second question: if Result will be "out" someday - yes, there should be a warning, like it is now for, say, Integer.
Alexander
A: 

If you think that some automatic management of strings are made to make your life easier, you're only partly right. All such things are also done to make string logic consistent and side-effects free.

In plenty of places there are string passed by reference, passed by value, but all these lines are expecting VALID strings in whose memory-management counter is some valid, not a garbage value. So in order to keep strings valid the only thing for sure is that they should be initialized when they firstly introduced. For example, for any local variable string this is a necessity since this is the place a string is introduced. All other string usage including function(): string (that actually procedure(var Result: string) as Alexander correctly pointed out) just expects valid strings on the stack, not initialized. And validness here comes from the fact that (var Result: string) construction says that "I'm waiting for a valid variable that definetly was introduced before". UPDATE: Because of that the actual contents of Result is unexpected, but due to the same logic, if it's the only call to this function that has a local variable at the left, the emptiness of the string in this case is guaranteed.

Maksee
+1  A: 

This is not a Bug. By definition no variable inside function is initialized, including Result.

So your Result is undefind on first call, and can hold anything. How it is implemented in compiler is irrelevant, and you can have different results in different compilers.

dmajkic
A: 

It seems like your function should be simplified like this:

function TMyObject.GenerateInfo: string;
begin
  if(ACondition) then
    Result := 'Some Text'
  else
    Result := '';
end;

You typically don't want to use Result on the right side of an assignment in a function.

Anyway, strictly for illustrative purposes, you could also do this, though not recommended:

procedure TForm1.Button1Click(Sender: TObject);
var
  i: integer;
  S: string;
begin
  for i := 1 to 5 do
  begin
    S := ''; // Clear before you call
    S := GenerateInfo;
  end;
  ShowMessage(S); // 5 lines!
end;
Marcus Adams