CODEWITHSUNDEEP
delphi
Franz
Franz

Reputation: 2049

parse a delphi text , Access violation with function call

I wanted to parse a ASCII Text for all occurences of e.g. ( bla, bla, bla ), text inside brackets using the following code 

function FilterText(const Source, TagStart, TagEnd: string): TstringList;  overload;
var StartPos, EndPos: integer;

begin
  result.Clear;
  try
    StartPos := PosEx(TagStart,Source);
    while StartPos > 0 do
      begin
        inc(StartPos,Length(TagStart));
        EndPos := PosEx(TagEnd,Source,StartPos);
        result.Add(Copy(Source,StartPos,EndPos - StartPos));
        StartPos := PosEx(TagStart,Source,EndPos + Length(TagEnd));
      end;

  finally

  end;
end;

I call this function using the following code

 memo1.Lines.AddStrings( FilterText(memo1.Lines.Text,'(',')'));

Instead of returning all text between brackets I get an AV .... why ????

Upvotes: 1

Views: 363

Answers (3)

David Heffernan
David Heffernan

Reputation: 613382

This line:

result.Clear;

Calls a method on an unitialized variable of type TStringList. That leads to undefined behaviour. In your case it seems that manifests as an access violation.

You could instantiate the string list inside FilterText, and so avoid the AV. 

function FilterText(...): TStringList;
begin
  result := TStringList.Create;
  try
    ....
  except
    result.Free;
    raise;
  end;
end;

But that leaves you with a bigger problem. Who is going to take ownership of the object? You use the function like this:

memo1.Lines.AddStrings(FilterText(memo1.Lines.Text,'(',')'));

Since your design has FilterText returning a new string list object, nothing takes ownership and therefore you leak.

You could do it like this:

FilteredText := FilterText(...);
try
  memo1.Lines.AddStrings(FilteredText); 
finally
  FilteredText.Free;
end;

Since FilterText is instantiating a new object and expecting the caller to take ownership, it would be prudent to give it a name to indicate that ownership transfer. For example, you might call the function CreateFilteredText.

Another option would be to expect the caller to instantiate the string list, and pass that object to FilterText. I personally find that approach to be a little easier to understand.

That version might look like this:

procedure FilterText(..., FilteredText: TStrings);
begin
  ...
  Strings.Add(...);
  ...
end;

And from the caller's side:

FilteredText := TStringList.Create;
try
  FilterText(..., FilteredText);
  memo1.Lines.AddStrings(FilteredText); 
finally
  FilteredText.Free;
end;

Upvotes: 3

MBo
MBo

Reputation: 80287

You forgot to create Result (TStringList).
You can also use TStrings (more general type than TStringList) argument instead of result like

procedure FilterText(const Source, TagStart, TagEnd: string; AStrings: TStrings);

so external code will responsible both for creation and for deletion of list.

Upvotes: 1

Ken White
Ken White

Reputation: 125749

Your function returns a TStringList, but never creates it. (It clears it instead, which is not necessary.)

begin
  Result := TStringList.Create;
  ...
end;

I personally don't like this, as it means the memory is allocated in the function but has to be freed by the code calling it. I prefer to have the stringlist passed in as a parameter to the function instead, so that it's clear that the caller both has to allocate it and free it.

Upvotes: 1

Related Questions