Add const char * overloads to String class

[Rubonnek]

The added function overloads, namely:

int _count(const char *p_string, int p_from, int p_to, bool p_case_insensitive) const;
int rfind(const char *p_str, int p_from = -1) const;
int rfindn(const char *p_str, int p_from = -1) const;
String replace_first(const char *p_key, const char *p_with) const;
String replacen(const char *p_key, const char *p_with) const;
int count(const char *p_string, int p_from = 0, int p_to = 0) const;
int countn(const char *p_string, int p_from = 0, int p_to = 0) const;

help avoid creating a ton of String objects and the underlying dynamic memory allocations they require by using the C-strings directly when possible.

I personally find that this patch makes the Editor feel snappier, but I have not run any benchmarks yet.

Edit: Added the following overloads:

int String::findn(const char *p_str, int p_from)
bool ends_with(const char *p_string) const;
String trim_prefix(const char *p_prefix) const;
String trim_suffix(const char *p_suffix) const;
int get_slice_count(const char *p_splitter) const;
String get_slice(const char *p_splitter, int p_slice) const;
Vector<String> split(const char *p_splitter = "", bool p_allow_empty = true, int p_maxsplit = 0) const;
Vector<String> rsplit(const char *p_splitter = "", bool p_allow_empty = true, int p_maxsplit = 0) const;

Edit: In #84379 (comment) @miv391 pointed out that finding an empty string ("") through String::find(const char *p_str, int p_from) returns 0 which is inconsistent when doing the same through GDScript. I've updated this PR to also fix that pre existing bug since it should return -1.

[Rubonnek]

I noticed I missed another overload I wanted and took the liberty to add int String::findn(const char *p_str, int p_from) as well, which is the last one I'll add for this PR.

[AThousandShips]

Let's add some unit tests for these to ensure correctness, do they work with unicode? If so then ensure it works with non-utf8 input

Edit: or rather, you need to rework the tests to test for the String and c-string versions, because the current tests use string literals that are now going through a different method

[AThousandShips]

It currently does break current checks, and unsure what happens with various cases using non-ascii/utf8 input

[Rubonnek]

Edit: or rather, you need to rework the tests to test for the String and c-string versions, because the current tests use string literals that are now going through a different method

Indeed. I was about to ask about this -- I'm not sure if we want to duplicate the tests like this:

	CHECK(s.findn("WHA") == 7);
	CHECK(s.findn(String("WHA")) == 7);

Or maybe add a macro to test both under the same line and keep it DRY.

I don't know if there's a cleaner way to do this. There doesn't seem to be.

[AThousandShips]

Haven't directly tested, and there is the possibility of code breaking with non-ascii strings passed as literals, but this applies to the already added methods like find, so not a new issue, would be good to confirm that it doesn't break things like: str.findn(U"...")

Otherwise it looks good to me, but TIWAGOS

[Rubonnek]

would be good to confirm that it doesn't break things like: str.findn(U"...")

I should clarify I intended to keep this PR unrelated to U"" and L"" C-strings. Those calls won't break due to this PR because they are on an unrelated code path. The call str.findn(U"") yields a function signature similar to findn(const char32_t*) for which there is no implementation (this PR only implements findn(const char *)) and as a result the compiler will look for other possible function candidates it can call given the findn(const char32_t*) signature. Currently, there's only one other candidate which is str.findn(String) which the compiler knows how to call because it can convert the const char32_t* parameter to a String through the String copy constructor String::String(const char32_t*). The same happens with a str.findn(L"") function call and, again, this PR never touches either of those code paths.

Having said that, we are indeed missing some tests for U"" and L"" C-strings. I'll add the tests for the functions I've overloaded if they pass.

[AThousandShips]

Agreed, just wanted to make sure the automatic overload resolution works correctly as C++ can be so messy with that some times

Tests would be great to ensure it (and future changes)

[Rubonnek]

Tests passed for U"" and L"" C-strings on my end so I've added them.

[AThousandShips]

Realized some missed details

Also needs some fixed for casting

[akien-mga]

I personally find that this patch makes the Editor feel snappier, but I have not run any benchmarks yet.

Would be nice to do some benchmarking on an optimized build to see if there is actually a significant difference. Performance optimization can have surprising results only reasoned about but not tested.

CC @Calinou

[Calinou]

Would be nice to do some benchmarking on an optimized build to see if there is actually a significant difference. Performance optimization can have surprising results only reasoned about but not tested.

Here are benchmarks of editor startup + shutdown time (I ran these twice and results were consistent):

❯ hyperfine -iw1 "bin/godot.linuxbsd.editor.x86_64.master --path /tmp/4 -e --quit" "bin/godot.linuxbsd.editor.x86_64 --path /tmp/4 -e --quit"
Benchmark 1: bin/godot.linuxbsd.editor.x86_64.master --path /tmp/4 -e --quit
  Time (mean ± σ):      3.027 s ±  0.058 s    [User: 2.075 s, System: 0.361 s]
  Range (min … max):    2.911 s …  3.132 s    10 runs
 
Benchmark 2: bin/godot.linuxbsd.editor.x86_64 --path /tmp/4 -e --quit           <--- This PR
  Time (mean ± σ):      2.982 s ±  0.290 s    [User: 2.074 s, System: 0.366 s]
  Range (min … max):    2.523 s …  3.537 s    10 runs
 
Summary
  bin/godot.linuxbsd.editor.x86_64 --path /tmp/4 -e --quit ran
    1.02 ± 0.10 times faster than bin/godot.linuxbsd.editor.x86_64.master --path /tmp/4 -e --quit

The difference is small, but it's an improvement either way.

As for binary size:

Editor

master: 105,335,696 bytes
    PR: 105,270,128 bytes (-65 KB)

Release export template

master: 62,385,544 bytes
    PR: 62,344,616 bytes (-41 KB)

I think the improvement is worth the added code complexity, as getting binary size down is important.

[Rubonnek]

Rebased to incorporate test changes from #89194.

[miv391]

How about some more tests for finds. I would also suggest separating find and rfind test to separate test cases (also findn and rfindn).

These tests are still in the old format.

	CHECK(s.find("") == 0);
	CHECK(s.find("", 9) == 9);
	CHECK(s.find("Pretty Woman Woman") == 0);
	CHECK(s.find("WOMAN") == -1);

	CHECK(s.rfind("") == 18); // fails (returns -1)
	CHECK(s.rfind("", 15) == 15); // fails (returns -1)
	CHECK(s.rfind("foo") == -1);
	CHECK(s.rfind("Pretty Woman Woman") == 0);
	CHECK(s.rfind("man", 13) == 9);
	CHECK(s.rfind("WOMAN") == -1);

	CHECK(s.findn("") == 0); // fails (returns -1)
	CHECK(s.findn("", 3) == 3); // fails (returns -1)
	CHECK(s.findn("wha") == 7);
	CHECK(s.findn("Wha") == 7);

	CHECK(s.rfindn("") == 15); // fails (returns -1)
	CHECK(s.rfindn("", 13) == 13); // fails (returns -1)
	CHECK(s.rfindn("wha") == 13);
	CHECK(s.rfindn("Wha") == 13);

There are some some inconsistencies how things work in master and thus some of these tests fail. Finding with "" seems to be a somewhat undefined case. The basic find returns 0 when "" is searched, which to me seems to be the correct answer. There is a "" at every index. But all other functions (rfind, findn, rfindn) return -1 when "" is searched. In my opinion that is wrong, when "" is searched, I would expect the first index checked to be returned.

However, in GDScript both find and rfind return -1 when "" is searched. Although I personally would prefer not returning -1, consistency is more important. If it is decided that "" is never found (-1), current C++ find implementation should be changed.

[Rubonnek]

The basic find returns 0 when "" is searched, which to me seems to be the correct answer. There is a "" at every index. But all other functions (rfind, findn, rfindn) return -1 when "" is searched.

That's a bug in the overload String::find(const char *p_str, int p_from). It's documented it should return -1, and the same applies to rfind, findn, and rfindn. The way I see it, "" is by definition an empty string and therefore searching for it implies nothing will be found and therefore the function should return -1.

I've patched this bug in this PR -- find("") now returns -1 to match the behavior of the rest. Thanks for pointing that out.

separate test cases

That's a style code change better suited for a different PR. I consider that change to be out of the scope of this PR.

[akien-mga]

Seems good to me. Could you rebase to make sure it still passes CI?

[Rubonnek]

CI checks passed after rebasing.

[akien-mga]

Thanks!