Jump to content
Sign in to follow this  
Cristian Peța

(Mis-)Behaviour of TStringHelper

Recommended Posts

13 hours ago, FredS said:

Essentially all system wide methods are run through DUnitX now.
This is how I know that RSP-15040 is still in Rio 10.3.1, I use that when anyone asks why I built my own string helper.

You haven't told what exactly is wrong and for me is working as expected.

Sincerely I don't have an hour to lose to identify what's wrong. Specifically what should be expected?

There is only one "Huh?" at "s.LastDelimiter('Hello')" but it returns 53 that is good. What's wrong?

 

P.S. The truth is that Delphi's LastIndexOf is not documented and is working but not as implemented in other languages.

Edited by Cristian Peța
P.S.

Share this post


Link to post
10 hours ago, Tommi Prami said:

That issue is closed, is there duplicate, or why is that? Any idea? Issue states fixed in 10.2 if I understand...

 

10 hours ago, Cristian Peța said:

You haven't told what exactly is wrong and for me is working as expected.

Sincerely I don't have an hour to lose to identify what's wrong. Specifically what should be expected?

There is only one "Huh?" at "s.LastDelimiter('Hello')" but it returns 53 that is good. What's wrong?

 

P.S. The truth is that Delphi's LastIndexOf is not documented and is working but not as implemented in other languages.

 

I didn't want to hijack this thread for something I moved on from years ago, so really quickly:


Berlin Help:  Returns the last index of the Value string in the current 0-based string.

  { TStringHelper.LastIndexOf returns wrong values } 
  //    0                 19                 38                  0-based
  s := 'Hello how are you, Hello how are you, Hello how are you';
  Assert(s.IndexOf('Hello', 38) = s.LastIndexOf('Hello', 38));

As for the "Huh?" next to LastDelimiter, that was me not understanding how that worked.

 

Share this post


Link to post
13 hours ago, FredS said:

  { TStringHelper.LastIndexOf returns wrong values } 
  //    0                 19                 38                  0-based
  s := 'Hello how are you, Hello how are you, Hello how are you';
  Assert(s.IndexOf('Hello', 38) = s.LastIndexOf('Hello', 38));

 

Wrong test case.

s.LastIndexOf('Hello', 38) will search starting from 38 to the left. That means in this string 'Hello how are you, Hello how are you, H'.

First occurrences is at 19 so it "Works As Expected".

I know this is not as other implementations but you must specify this in report if you want a change.

And this change can brake old code so it must be strongly justified.

  • Like 3

Share this post


Link to post
10 hours ago, Cristian Peța said:

s.LastIndexOf('Hello', 38) will search starting from 38 to the left. That means in this string 'Hello how are you, Hello how are you, H'.

First occurrences is at 19 so it "Works As Expected". 

 

I moved on in 2016, so what happens with this method is no longer for me only for those who spend hours searching for bugs they can't figure out.
And I'm not looking for the 'First occurrence', that would be 'IndexOf', so either way the function fails.

Returns the last index of the Value string in the current 0-based string.
StartIndex specifies the offset in this 0-based string where the LastIndexOf method begins the search, and Count specifies the end offset where the search ends.
There are six LastIndexOf overloaded methods, each one allowing you to specify various options in order to obtain the last index of the given string in this 0-based string.
When the Value argument (Char or String), passed to LastIndexOf, is not found in the 0-based string, LastIndexOf function returns -1.

 

 

 

 

Share this post


Link to post

The documentation is not precise enough - but fwiw the implementation as in Delphi is the same as it is in .NET or Java. The startindex actually also limits the length of the searched string. So any search text longer than 1 will only be found at startIndex - (Length(searchText) - 1) or left to that. And when Cristian said "first occurence" he actually meant "first occurence from the right" of course.

Edited by Stefan Glienke

Share this post


Link to post
5 hours ago, Stefan Glienke said:

The documentation is not precise enough

 

True but it is the same as IndexOf, should I now file a bug report for IndexOf?

 

StartIndex specifies the initial offset in this 0-based string where the search starts

Because this does not compute:

Assert(s.IndexOf('Hello', 38) = s.LastIndexOf('Hello', 38));

I originally looked at the source and also took " startIndex - (Length(searchText) - 1)" into account but here is that math:

  //    0                 19                 38                  0-based
  s := 'Hello how are you, Hello how are you, Hello how are you';
  //                                    ^ 32                     Startindex - (Length('Hello')- 1), searched forward or backward?

IMO at this point the claim that it works as expected needs to come with new documentation or a bug fix.

 

Share this post


Link to post

Sorry I meant

startIndex + Length(searchText) - 1

because the method simply works as follows: it starts to search at startIndex but does not consider any character in the string after that. That causes it not finding any searched text that extends beyond that index.

So as Christian already stated it basically works as if you cut the string after startIndex.

 

So what the documentation is simply missing is the fact that it would not find any occurence that extends past startIndex when you use any of those overloads, that's all.

Edited by Stefan Glienke

Share this post


Link to post

 

13 minutes ago, Stefan Glienke said:

it basically works as if you cut the string after startIndex

 

Yeah, certainly made clear in the documentation </sarcasm>

One would hope that when they copied a DotNet function they could at least copy the DotNet explanation:

The search starts at a specified character position and proceeds backward toward the beginning of the string for the specified number of character positions.

At this point the claim that it works as expected needs to come with new documentation.

  • Like 1

Share this post


Link to post

It's a bullsh@t. The doc is a big ~, the mimicked java/.net implementation is a ~, and the parameter name "startIndex" is a ~. It should be called lastIndex as the function name reflects only an index lookup, and not any search in some direction, and mostly not the underlying implementation.

Edited by Attila Kovacs

Share this post


Link to post
2 hours ago, Attila Kovacs said:

should be called lastIndex

LastIndex and DownRange would help but honestly, this is like having to flip your steering wheel when you shift the car into reverse. 🙂

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  

×