[stdlib] Fix String.split() implementations

[martinvuyk]

Main issue

Fix String.split() implementations to use a generic implementation and without assuming that indexing is by byte offset. Added all methods to StringLiteral and StringSlice. Some important optimizations were added by parametrizing and avoiding slicing with numeric tricks.

Changes in behavior

This PR changes split("") behavior to be non-raising and return the separated unicode characters analogous to when the whole string has the separator at start, end, and in between every character. Closes #3635

String, StringLiteral, and StringSlice .split() now return a List[StringSlice].

Benchmark results:

CPU: Intel® Core™ i7-7700HQ

improvement metric: markdown percentage improvement ((old_value - new_value) / old_value)

Average improvement for split with a sequence: 91.2486% . In orders of magnitude, this is a 11x improvement
Average improvement for split on any whitespace: 99.9975% . In orders of magnitude, this is a 40k x improvement

Name	old_value (ms)	new_value (ms)	improvement
bench_string_split[1000000]	5.6970877871796	0.503624904955618	0.912486298354641
bench_string_split_none[1000000]	147157.6808882	3.78480079182398	0.999975287456096

[msaelices]

Great job. I've just add a NIT-pick suggestion.

Also, is it possible to add a unit test?

[martinvuyk]

Great job. I've just add a NIT-pick suggestion.

Also, is it possible to add a unit test?

Hi, thanks for the review. Any type of test in mind that split tescases don't cover ?

[msaelices]

Great job. I've just add a NIT-pick suggestion.
Also, is it possible to add a unit test?

Hi, thanks for the review. Any type of test in mind that split tescases don't cover ?

I thought this check would be broken in nightly but I am glad that it's not:

❯ git diff
diff --git a/stdlib/test/collections/test_string.mojo b/stdlib/test/collections/test_string.mojo
index a664d321..b7a85c6c 100644
--- a/stdlib/test/collections/test_string.mojo
+++ b/stdlib/test/collections/test_string.mojo
@@ -824,6 +824,11 @@ def test_split():
     assert_equal(res6[2], "долор")
     assert_equal(res6[3], "сит")
     assert_equal(res6[4], "амет")
+    var res7 = in6.split("м")
+    assert_equal(res7[0], "Лоре")
+    assert_equal(res7[1], " ипсу")
+    assert_equal(res7[2], " долор сит а")
+    assert_equal(res7[3], "ет")

BTW, I still think it's a good test to add.

[martinvuyk]

I thought this check would be broken in nightly but I am glad that it's not:

❯ git diff
diff --git a/stdlib/test/collections/test_string.mojo b/stdlib/test/collections/test_string.mojo
index a664d321..b7a85c6c 100644
--- a/stdlib/test/collections/test_string.mojo
+++ b/stdlib/test/collections/test_string.mojo
@@ -824,6 +824,11 @@ def test_split():
     assert_equal(res6[2], "долор")
     assert_equal(res6[3], "сит")
     assert_equal(res6[4], "амет")
+    var res7 = in6.split("м")
+    assert_equal(res7[0], "Лоре")
+    assert_equal(res7[1], " ипсу")
+    assert_equal(res7[2], " долор сит а")
+    assert_equal(res7[3], "ет")

BTW, I still think it's a good test to add.

I'm not understanding, so the lines from var res7 ... onwards didn't get merged in another PR and you'd like me to add them here?

[msaelices]

I thought this check would be broken in nightly but I am glad that it's not:

❯ git diff
diff --git a/stdlib/test/collections/test_string.mojo b/stdlib/test/collections/test_string.mojo
index a664d321..b7a85c6c 100644
--- a/stdlib/test/collections/test_string.mojo
+++ b/stdlib/test/collections/test_string.mojo
@@ -824,6 +824,11 @@ def test_split():
     assert_equal(res6[2], "долор")
     assert_equal(res6[3], "сит")
     assert_equal(res6[4], "амет")
+    var res7 = in6.split("м")
+    assert_equal(res7[0], "Лоре")
+    assert_equal(res7[1], " ипсу")
+    assert_equal(res7[2], " долор сит а")
+    assert_equal(res7[3], "ет")

BTW, I still think it's a good test to add.

I'm not understanding, so the lines from var res7 ... onwards didn't get merged in another PR and you'd like me to add them here?

It's just a diff if you want to complete it with more test. LGTM anyways so don't worry. Thanks for that contribution 🥇

[martinvuyk]

@JoeLoser can we merge this? 5 months of keeping this on top of the tree and with so many changes to string stuff is laborious.

@ConnorGray I'm un-deprecating __iter__ here because it is needed and is simply an iterator over codepoint slices. Here you can also see many examples where the performance of building a Codepoint (encoding from utf8 to utf32) would be unnecessarily expensive.

My plan is to then on top of this, similar to #3858, build an iterator taking this code. Then just adding a layer which assembles it into a list.