1

The function below takes in XML input, parses it, and returns an ordinary string which is simply displayed in the caller function. So objects in the context are internal to the function below.

But this function has a strange problem that, it remembers the input which means that the objects are not released properly. Output string has a part resulting from the previous call even if the input is checked to be different.

Before assigning to nil each of XMLDoc, IXMLNodeList, I also tried to release, in a loop, each IXMLNode directly referred from the Item array by assigning nil to it but that assignment statement resulted in a syntax error so settled for the below.

function tform1.nodeToSentence(nodeXml: string): string ;
var
  tempXmlDoc : IXMLDocument;
  resultWordPuncNodes : IXMLNodeList;
  i: Integer;

begin
  tempXmlDoc := CreateXMLDoc;
  tempXmlDoc.LoadXML(nodeXml);
  resultWordPuncNodes := XPathSelect(tempXmlDoc.DocumentElement,'//*');

  for i:= 0 to resultWordPuncNodes.Length-1   do
  begin

    if resultWordPuncNodes.Item[i].NodeName = 'name' then
     begin
       if resultWordPuncNodes.Item[i].Attributes['attrib'] = 'v_attrib' then
       begin
          Result := TrimRight(Result);
          Result := Result + resultWordPuncNodes.Item[i].Text + ' ';
       end
       else Result := Result + resultWordPuncNodes.Item[i].Text + ' ';
     end;
  end;


  resultWordPuncNodes:=nil;
  tempXmlDoc := nil; //interface based objects are freed this way
end;

calling code

//iterating over i   
          nodeXML := mugList.Strings[i];
          readableSentence := nodeToSentence(mugList.Strings[i]);
          //examplesList.Append(readableSentence);
          //for debugging
          showMessage(readableSentence);
 //iteration ends
user30478
  • 347
  • 1
  • 3
  • 13

2 Answers2

2

But this function has a strange problem that, it remembers the input which means that the objects are not released properly. Output string has a part resulting from the previous call even if the input is checked to be different.

That is because a String return value of a function (and also other ARC-managed types, as well as record, object, and method pointers) gets passed between caller and callee using a hidden var parameter that is NOT automatically cleared when the function is entered, as you are expecting.

This code:

function tform1.nodeToSentence(nodeXml: string): string ;
...
readableSentence := nodeToSentence(mugList.Strings[i]);

Is effectively the same as this code:

procedure tform1.nodeToSentence(nodeXml: string; var Result: string);
...
nodeToSentence(mugList.Strings[i], readableSentence);

Call nodeToSentence() multiple times, and you are potentially appending more and more text to the same String variable.

Inside of the function, you need to manually reset the Result value before you start concatenating new values to it:

function tform1.nodeToSentence(nodeXml: string): string ;
var
  ...
begin
  Result := ''; // <-- add this!
  ...
end;
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Remy, please. 1) do not edit my code. 2) do not edit it while I am being editing it. 3) do not change a correct code into a broken one. Assigning String variable to nil would not even compile! – Arioch 'The Dec 07 '16 at 20:20
  • 3
    The **only** thing I changed was to make `Result := nil` be `Result := ''` instead. As you yourself stated, "*You can not assign NIL to a string variable*", so I was simply fixing a bug for you. – Remy Lebeau Dec 07 '16 at 20:23
  • Okay, I misread the editing logs. Indeed, #3 was my blunder, sorry. But the rest holds, while I was making a long edit SO suddenly tells me I can not save it. Is was annoying. A little comment here would suffice :-) – Arioch 'The Dec 07 '16 at 20:25
  • @Arioch'The: I was going to make a lot more edits, because frankly, your answer is quite hard to read. But I refrained and posted my own answer instead. – Remy Lebeau Dec 07 '16 at 20:27
  • 1
    Well, doing "lot more" edits, while the person is doing his own is... shady. Also my answers give more information and touches more issues. You are keeping extending your own initially short answer too, I see – Arioch 'The Dec 07 '16 at 20:29
  • Actually I do not remember if this "not really a function" only happens with string type or any ARC type including intfs, lambdas and dynarrays.... I imagine what a mess can lambda-generator "function" with such an oversight cause.... – Arioch 'The Dec 07 '16 at 20:38
  • 1
    @Arioch'The: it happens with all ARC types, to ensure proper ARC semantics. In case the caller does not assign the result to a real variable, a hidden variable is used. – Remy Lebeau Dec 07 '16 at 20:55
  • @RemyLebeau This was a very readable answer indeed! – user30478 Dec 07 '16 at 20:57
  • @RemyLebeau I think thsio hack destroys ARC semantics, not maintains it. And you see, one more user got himself hoisted on this old landmine... Just indeed imagine a multistep lambda generator like that. `if Flag1 in Flags then Result := function (x: string):string begin Exit(TrimLeft (x)) end; if Flag2 in Flags then Result := function (x: string):string begin Exit(TrimRight (x)) end; ... ` - it would be very susceptible to the same trap. Delphi has to implement OUT-semantics for ARC-types, not merely VAR-semantics. It would only need a single INITIALIZE hidden call... – Arioch 'The Dec 07 '16 at 21:08
1

It does not have to do with interfaces. It only has with string implementation in Delphi.

You have to clear the Result variable as the first line of your function.

Here below is your function fixed:

function tform1.nodeToSentence(nodeXml: string): string ;
var
  tempXmlDoc : IXMLDocument;
  resultWordPuncNodes : IXMLNodeList;
  i: Integer;

begin

// Change #1 - added the line
  Result := ''; 
// Variable Result is shared here before by both the function and the caller.
// You DO HAVE to clear the shared variable to make the function FORGET the previous result.
// You may do it by the 'function' or by the calling site, but it should have be done.
// Usually it is better to do it inside the function.

  tempXmlDoc := CreateXMLDoc;
  tempXmlDoc.LoadXML(nodeXml);
  resultWordPuncNodes := XPathSelect(tempXmlDoc.DocumentElement,'//*');

  for i:= 0 to resultWordPuncNodes.Length-1   do
  begin

    if resultWordPuncNodes.Item[i].NodeName = 'name' then
     begin
       if resultWordPuncNodes.Item[i].Attributes['attrib'] = 'v_attrib' then
       begin

/// REMEMBER this line, it is important, I would explain later.
          Result := TrimRight(Result);
          Result := Result + resultWordPuncNodes.Item[i].Text + ' ';
       end
       else Result := Result + resultWordPuncNodes.Item[i].Text + ' ';
     end;
  end;

  resultWordPuncNodes:=nil;

// Change #2 - removed the line - it is redundant
 (* tempXmlDoc := nil; //interface based objects are freed this way *)
// Yes, they are. But Delphi automatically clears local ARC-variables 
//   when destroying them where the function exits.
// Variable should both be local and should be one of ARC types foe that.
end;

1) about auto-clearing local and ARC-kind variables see http://docwiki.embarcadero.com/Libraries/XE8/en/System.Finalize

2) Your real function is not a function at all.

// function tform1.nodeToSentence(nodeXml: string): string ;   // only an illusion  
procedure tform1.nodeToSentence(nodeXml: string; var Result: string);  // real thing

You may say that you was writing a function, not the procedure. However that was merely a syntactic sugar. It is like TDateTime and double are different terms, but those terms are synonyms for the same implementation;

All the functions returning String/AnsiString/UnicodeString variables in Delphi are procedures. They are merely disguised as function, and the disguise is thin.

3) there is an old Delphi kōan about it. Without OmniXML and all the complex stuff that only blurs the truth. Run this code and see how it behaves.

Function Impossible: String;
begin
  ShowMessage( 'Empty string is equal to: ' + Result );
end;

Procedure ShowMe; Var spell: string;
begin
  spell := Impossible();

  spell := 'ABCDE';
  spell := Impossible();

  spell := '12345';
  spell := Impossible();
end;

4) Now, could you know it ? Yes you could, it only takes a little bit of attention. Let us make a little change.

Function ImpossibleS: String;
begin
  ShowMessage( 'Unknown string today is equal to: ' + Result );
end;

Function ImpossibleI: Integer;
begin
  ShowMessage( 'Unknown integer today is equal to: ' + IntToStr(Result) );
end;


Procedure ShowMe; 
Var spell: string; chant: integer;
begin
  spell := ImpossibleS();

  spell := 'ABCDE';
  spell := ImpossibleS();

  spell := '12345';
  spell := ImpossibleS();

  chant := ImpossibleI();

  chant := 100;
  chant := ImpossibleI();

  chant := 54321;
  chant := ImpossibleI();
end;

Be attentive and read compilation warnings. You did fixed your original code that it compiles with NO COMPILER WARNINGS there, didn't you ?

Now compile my second kōan. Read the warnings. Integer-function does generate "Non-initialized variable" warning. String-function does not. Is it so ?

Why the difference? Because string is ARC-type. That means string-function is really a procedure, not a function. And that means the Result variable is initialized, by the caller, outside of the looks-like-function. You may also observe that chant variables does change after the call, because the integer-function is a real function and the assignment-after-call is a real one. Conversely a string-function is an illusionary one and so is the illusionary assignment-after-call. It looks as being assigned, but it is not.

5) Last thing. Why did I put that mark in your code.

/// REMEMBER this line, it is important, I would explain later.
          Result := TrimRight(Result);

Exactly because of that kōan above. You must have been reading the garbage here. You must have been using "Result" variable that you did not initialized nowhere before. You must have expected your compiler giving warning to you at this line. Like it does in ImpossibleI real function above. But it does not. Just like it does not with ImpossibleS illusionary function. This lack of the warning is a dead giveaway of the illusion Delphi creates here. Would you notice that there SHOULD be a 'non-initialized var' warning but it got missing you would ask yourself "who initialized variable if it was not my function" and you would understand what had happened yourself. ;-)

6) Okay, one more last thing.

procedure StringVar( const S1: string; var S2: string );
begin
  ShowMessage ( S1 + S2 );
end;

procedure StringOut( const S1: string; out S2: string );
begin
  ShowMessage ( S1 + S2 );
end;

Those two procedures looks similar. The difference is S2 kind. It should be an OUT-parameter, not the VAR (IN+OUT) parameter. But Delphi fails here just with your function. Delphi can not do string-type OUT-parameters. To compare, FreePascal(Lazarus,CodeTyphon) knows the difference and would show the legit 'non-initialized variable' warning for the StringOut procedure.

Arioch 'The
  • 15,799
  • 35
  • 62
  • If I don't care about the passed 'Result' argument's clearing in the callee function, doesn't it gets cleared in the first line (Result = nil)? – user30478 Dec 07 '16 at 20:10
  • 1
    You can not assign NIL to a string variable. Yes, it would be cleared if you assign to it. But you DID NOT assign to it. And that is it. If you want it to be cleared you have to clear it. Either inside the function or outside, but you have to do it. – Arioch 'The Dec 07 '16 at 20:11
  • 1
    Insight into the problem (string return actually a 'var' argument), Source of garbage(Result := TrimRight(Result)), hint about XMLDoc freeing, and all the fundamental stuff (koan approach, ARC-types, compiler warnings etc ) make it an EXCELLENT answer. It will be very helpful for future readers. – user30478 Dec 07 '16 at 20:51
  • 1
    Sadly we are getting more and more used to "eternal September" kind of readers. Those who come with "just fix the code, don't make me think, it hurts" attitude. It really is refreshing to see new readers who like to pro-actively think and explore. Hope you would finally get into a regular visitor of SO :-D – Arioch 'The Dec 07 '16 at 21:03
  • OTOH, future readers would never find it, they would think this issue is about OmniXML and just skip it, they would fall into this trap with some other code and libraries instead :-D – Arioch 'The Dec 07 '16 at 21:11
  • BTW, about koans, did you noted that on the first call to `spell := ImpossibleS();` the variable is really empty as it would be expected to be? But.... we did not initialized it, so who did? :-D – Arioch 'The Dec 07 '16 at 21:14
  • Any other possible title of the question? – user30478 Dec 07 '16 at 21:23
  • As far as the message output is concerned spell := impossibleS(); shows up empty string in the variable's place. – user30478 Dec 07 '16 at 21:34
  • Yes, it is empty. But why is it? The function does not clear it, so who did? – Arioch 'The Dec 07 '16 at 23:54