Engine: IndexOf add string index, count and case sensitivity params

[ericoporto]

fix #2487

• I don't remember how to deal with the previous script API being only 1 parameter in ags4 branch! (Should I also link to String::IndexOf^1 and then have both be able to set through ifdefs in the header? Or is this not needed in ags4 ? the defaults will keep them working but rebuild is still needed.
• I am not convinced that the final code is actually getting proper offset in unicode characters, even as is in the current branch, did we test this at some point?
• I stole bits of the substring function to make the substring that the IndexOf will apply to. I wasn't sure if I should error here too or just throw a warn and return -1.

[ivan-mogilko]

(Should I also link to String::IndexOf^1 and then have both be able to set through ifdefs in the header? Or is this not needed in ags4

I am not sure myself, but if need arises then this will be trivial to add another table entry.

[ivan-mogilko]

I am not convinced that the final code is actually getting proper offset in unicode characters, even as is in the current branch,

It looks like returning a correct result in the current branch, because it does this after finding a "needle":

*offs = 0;
int at = ustrlen(tempbuf1);

This puts a null terminator right where the "needle" was found, and calculates the ulength of the string before that.

You can do the same. Or you may try to optimize this, and add a separate version of ustrstr in Allegro code that also has a pointer to "index" as an argument, and fills in an index at which a substring was found.

[ericoporto]

Right, this reminds me, stupid question, but the returned index is relative to the 0 character of the original string right? Not the substring.

[ivan-mogilko]

Right, this reminds me, stupid question, but the returned index is relative to the 0 character of the original string right? Not the substring.

Yes, it should be an index in the full string; the input parameters are also indexes in full string, so return value is consistent with them.

[ericoporto]

ok, I think I fixed all things. I also ended adding a table entry for IndexOf^1 pointing to StrContains as it was.

Also I had a copy paste error that is why it wasn't working at all, fixed now.

Edit: ok, had also a logic issue that I fixed, forgot to test endIndex as -1...

[ivan-mogilko]

We must clarify what does "end index" mean in IndexOf: is it a last character of a searched range, or a first index after range. Right now the function works as if it were the index after. In other words startIndex is inclusive and endIndex is exclusive, the search is done in range [startIndex, endIndex). Is that a expected behavior?

If that's intended, then IndexOf may exit early with -1 result if startIndex == endIndex.

In regards to the code, I'd argue that for the new Substring function it may be better to have start and length parameters, that's common for "substring" functions.

EDIT: I did few basic tests, with unicode strings too, and this appears to work.

[ericoporto]

I think Length is less ambiguous than end index, wouldn't it be better to use it instead as input parameter?

[ivan-mogilko]

Personally I don't mind "end index" so long as it's clearly documented; and in C and C++ it's traditionally used as an exclusive range ending (i.e. how iterator parameters work).

@messengerbag, you have suggested this parameter, what do you think about this?

EDIT: I suppose that a use case is to search inside a portion of a string without having to create substrings, so we may look from that perspective.

[ericoporto]

I think C# uses a "count" which would be length in the C# String.IndexOf. There are a lot of overloads, so not so sure if there's one that would also use index instead.

[ivan-mogilko]

Alright, we may use "count" too. It's easy to convert to count if you know ending index.

BTW in C#'s example the "comparison type" is the last argument after startIndex and count.

[ericoporto]

BTW in C#'s example the "comparison type" is the last argument after startIndex and count.

Uhm, we also have it as last parameter in the other String methods we have, I think switching it here too for consistency may be a good idea. Do you agree?

[ivan-mogilko]

Maybe call this parameter "count"? Count is number of characters after index, length is a length of an object, like a string.

we also have it as last parameter in the other String methods we have, I think switching it here too for consistency may be a good idea. Do you agree?

Yes, I think that will be alright.

[ericoporto]

ok, just a check, if we are inverting and switching for count, I think 0 would be better than -1 to be evaluated as instead going through the full length. I think that would be easier to remember as default for it.

[ivan-mogilko]

if we are inverting and switching for count, I think 0 would be better than -1 to be evaluated as instead going through the full length.

That makes sense, because 0 is a useless "count" value for this operation.

[ericoporto]

ok, I think now all is correct!

[messengerbag]

@messengerbag, you have suggested this parameter, what do you think about this?

I suppose this has already been settled, but since I was asked, I personally find that endIndex is usually more convenient, and I think making it non-inclusive would be preferable. That is the way which, in my experience, most often allows you to simply use the variable value you already have, without any kind of arithmetic.

But obviously, the conversion between the alternatives is pretty trivial, so it's not a huge deal.

[ericoporto]

Yeah, I had made endIndex non inclusive and also because in my head it was more convenient but in the end I think using count is easier to understand, less ambiguous, plus I think it will be easier to figure it out that 0 causes it to run until string end.

I am trying to make the API easier to get by reading parameter names and the tooltip comment and I think this achieved design with count does make sense in a somewhat more intuitive way.

Edit: unless someone thinks count doesn't sound like the length of the substring and instead it means count of number of needles... :/