[mojo-stdlib] Implement compare operation for String #2378

zhoujingya · 2024-04-22T07:31:38Z

This PR refers to issue #2346 , though the string test is disables now, I still write test cases for those operators

gabrieldemarmiesse

What is usually done in this case is to make a private function that returns -1 (strictly lower), 0 (equal) or +1 (strictly greater) and call this function in each of the __gt__, __eq__, __lt__... methods. CPython implements a few comparaisons with this system and it's quite readable. This is only an idea, it can be implemented in other ways.

stdlib/test/builtin/test_string.mojo

stdlib/src/builtin/string.mojo

zhoujingya · 2024-04-23T07:53:12Z

I tried to implement compare operation for StringLiteral too, but StringLiteral use self defined _memcmp inside

stdlib/src/builtin/string.mojo

ConnorGray

Thanks for your contribution @zhoujingya! I had only one minor piece of feedback (using min), but I'll address that when I import this PR internally.

Signed-off-by: zhoujing <jing.zhou@terapines.com>

Moosems

Connor Gray liked it and LGTM

laszlokindrat

Thanks for the patch! Any chance you could implement this in StringRef, and delegate from here (and from StringLiteral) to there as well?

laszlokindrat · 2024-05-07T18:09:59Z

stdlib/src/builtin/string.mojo

+        Returns:
+            True if self are less equal other string.
+        """
+        return _str_compare(self, other) == 0 or _str_compare(self, other) == -1


Suggested change

return _str_compare(self, other) == 0 or _str_compare(self, other) == -1

return not self.__gt__(other)

laszlokindrat · 2024-05-07T18:10:38Z

stdlib/src/builtin/string.mojo

+        Returns:
+            True if self are less than other string.
+        """
+        return _str_compare(self, other) == -1


Suggested change

return _str_compare(self, other) == -1

return not self.__ge__(other)

laszlokindrat · 2024-05-07T18:12:50Z

stdlib/src/builtin/string.mojo

@@ -52,6 +52,23 @@ fn _ctlz(val: SIMD) -> __type_of(val):
    )


+@always_inline
+fn _str_compare(str1: String, str2: String) -> Int:


Can you please provide a summary of what the logic is in the docstring? Also, please document that this can return {-1, 0, 1}.

laszlokindrat · 2024-05-07T18:15:52Z

stdlib/src/builtin/string.mojo

+        Returns:
+            True if self are greater equal the other string.
+        """
+        return _str_compare(self, other) == 0 or _str_compare(self, other) == 1


Please ensure _str_compare is called only once.

Suggested change

return _str_compare(self, other) == 0 or _str_compare(self, other) == 1

return _str_compare(self, other) != -1

laszlokindrat · 2024-05-07T18:16:34Z

stdlib/test/builtin/test_string.mojo

+    assert_true(str("feefef") > str("feefe"))
+    assert_false(str("hello") > str("hello"))
+    assert_true(str("hello") >= str("hello"))
+    assert_false(str("hello") < str("hello"))
+    assert_true(str("hello") <= str("hello"))
+    assert_false(str("apple") > str("banana"))
+    assert_false(str("apple") > str("banana"))
+    assert_true(str("apple") > str("Banana"))
+    assert_false(str("apple123") > str("apple456"))
+    assert_false(str("appl2$") > str("banana"))
+    assert_false(str("") > str("a"))


Can you please modify these tests so that they use the dunder methods directly (as opposed to the operators).

laszlokindrat · 2024-05-09T15:48:31Z

@zhoujingya Thanks for the patch! Are you still interested in pushing this forward?

laszlokindrat · 2024-05-10T21:21:08Z

Closing this in favor of #2409.

[External] [stdlib] String comparisons implemented For issue modularml#2346 (as an alternative to modularml#2378). All four comparisons (`__lt__`, `__le__`, `__gt__`, & `__ge__`) uses a single `__lt__` comparison (instead of checking less/greater than + potentially another "equals to"-check, for `__le__` & `__ge__`). Sorry if this is considered a duplicate PR, I only meant to give an alternative suggestion. This is my first ever PR on GitHub. StringLiterals also get comparisons. ORIGINAL_AUTHOR=Simon Hellsten <56205346+siitron@users.noreply.github.com> PUBLIC_PR_LINK=modularml#2409 --------- Co-authored-by: Simon Hellsten <56205346+siitron@users.noreply.github.com> Closes modularml#2409 MODULAR_ORIG_COMMIT_REV_ID: b2ed4756c2741fd27387fa295515f4a7222e0ca5

[External] [stdlib] String comparisons implemented For issue #2346 (as an alternative to #2378). All four comparisons (`__lt__`, `__le__`, `__gt__`, & `__ge__`) uses a single `__lt__` comparison (instead of checking less/greater than + potentially another "equals to"-check, for `__le__` & `__ge__`). Sorry if this is considered a duplicate PR, I only meant to give an alternative suggestion. This is my first ever PR on GitHub. StringLiterals also get comparisons. ORIGINAL_AUTHOR=Simon Hellsten <56205346+siitron@users.noreply.github.com> PUBLIC_PR_LINK=#2409 --------- Co-authored-by: Simon Hellsten <56205346+siitron@users.noreply.github.com> Closes #2409 MODULAR_ORIG_COMMIT_REV_ID: b2ed4756c2741fd27387fa295515f4a7222e0ca5

zhoujingya requested a review from a team as a code owner April 22, 2024 07:31

zhoujingya mentioned this pull request Apr 22, 2024

[Feature Request] More precise assertation failure information need to be provided #2379

Closed

1 task

gabrieldemarmiesse reviewed Apr 22, 2024

View reviewed changes

stdlib/test/builtin/test_string.mojo Outdated Show resolved Hide resolved

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved

soraros reviewed Apr 22, 2024

View reviewed changes

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved

zhoujingya requested review from gabrieldemarmiesse, Moosems and soraros April 23, 2024 05:44

mzaks reviewed Apr 23, 2024

View reviewed changes

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved

soraros reviewed Apr 23, 2024

View reviewed changes

stdlib/src/builtin/string.mojo Outdated Show resolved Hide resolved

zhoujingya force-pushed the string-compare branch from acb77b1 to 2c0cae6 Compare April 23, 2024 09:10

zhoujingya requested a review from soraros April 24, 2024 01:00

siitron mentioned this pull request Apr 25, 2024

[stdlib] String comparisons implemented #2409

Closed

ConnorGray reviewed May 1, 2024

View reviewed changes

stdlib/src/builtin/string.mojo Show resolved Hide resolved

ConnorGray approved these changes May 1, 2024

View reviewed changes

ConnorGray added the imported-internally Signals that a given pull request has been imported internally. label May 1, 2024

[mojo-stdlib] Implement compare operation for String

16ae793

Signed-off-by: zhoujing <jing.zhou@terapines.com>

ConnorGray force-pushed the string-compare branch from 2c0cae6 to 16ae793 Compare May 1, 2024 21:03

Moosems approved these changes May 1, 2024

View reviewed changes

mzaks mentioned this pull request May 5, 2024

[Feature Request] Adding a builtin sort/sorted function #2390

Open

1 task

ematejska added the mojo-repo Tag all issues with this label label May 6, 2024

laszlokindrat reviewed May 7, 2024

View reviewed changes

laszlokindrat self-assigned this May 9, 2024

mzaks mentioned this pull request May 10, 2024

[stdlib] Introduce alternative list sorting for scalar and order comparable elements #2609

Open

laszlokindrat closed this May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mojo-stdlib] Implement compare operation for String #2378

[mojo-stdlib] Implement compare operation for String #2378

zhoujingya commented Apr 22, 2024

gabrieldemarmiesse left a comment

zhoujingya commented Apr 23, 2024

ConnorGray left a comment

Moosems left a comment

laszlokindrat left a comment

laszlokindrat May 7, 2024

laszlokindrat May 7, 2024

laszlokindrat May 7, 2024

laszlokindrat May 7, 2024

laszlokindrat May 7, 2024

laszlokindrat commented May 9, 2024

laszlokindrat commented May 10, 2024

	return _str_compare(self, other) == 0 or _str_compare(self, other) == -1
	return not self.__gt__(other)

	return _str_compare(self, other) == -1
	return not self.__ge__(other)

	return _str_compare(self, other) == 0 or _str_compare(self, other) == 1
	return _str_compare(self, other) != -1

[mojo-stdlib] Implement compare operation for String #2378

[mojo-stdlib] Implement compare operation for String #2378

Conversation

zhoujingya commented Apr 22, 2024

gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

zhoujingya commented Apr 23, 2024

ConnorGray left a comment

Choose a reason for hiding this comment

Moosems left a comment

Choose a reason for hiding this comment

laszlokindrat left a comment

Choose a reason for hiding this comment

laszlokindrat May 7, 2024

Choose a reason for hiding this comment

laszlokindrat May 7, 2024

Choose a reason for hiding this comment

laszlokindrat May 7, 2024

Choose a reason for hiding this comment

laszlokindrat May 7, 2024

Choose a reason for hiding this comment

laszlokindrat May 7, 2024

Choose a reason for hiding this comment

laszlokindrat commented May 9, 2024

laszlokindrat commented May 10, 2024