-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mojo-stdlib] Implement compare operation for String #2378
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I tried to implement compare operation for |
acb77b1
to
2c0cae6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connor Gray liked it and LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Any chance you could implement this in StringRef
, and delegate from here (and from StringLiteral
) to there as well?
Returns: | ||
True if self are less equal other string. | ||
""" | ||
return _str_compare(self, other) == 0 or _str_compare(self, other) == -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return _str_compare(self, other) == 0 or _str_compare(self, other) == -1 | |
return not self.__gt__(other) |
Returns: | ||
True if self are less than other string. | ||
""" | ||
return _str_compare(self, other) == -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return _str_compare(self, other) == -1 | |
return not self.__ge__(other) |
@@ -52,6 +52,23 @@ fn _ctlz(val: SIMD) -> __type_of(val): | |||
) | |||
|
|||
|
|||
@always_inline | |||
fn _str_compare(str1: String, str2: String) -> Int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please provide a summary of what the logic is in the docstring? Also, please document that this can return {-1, 0, 1}.
Returns: | ||
True if self are greater equal the other string. | ||
""" | ||
return _str_compare(self, other) == 0 or _str_compare(self, other) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure _str_compare
is called only once.
return _str_compare(self, other) == 0 or _str_compare(self, other) == 1 | |
return _str_compare(self, other) != -1 |
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")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please modify these tests so that they use the dunder methods directly (as opposed to the operators).
@zhoujingya Thanks for the patch! Are you still interested in pushing this forward? |
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
[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 Signed-off-by: Lukas Hermann <lukashermann28@gmail.com>
[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
This PR refers to issue #2346 , though the string test is disables now, I still write test cases for those operators