-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[stdlib] Delegate string comparisons to StringRef
#2620
Conversation
…ring and StringLiteral are delegated to StringRef. Tests for StringRef comparisons added. Signed-off-by: Simon Hellsten <simonhellsten@hotmail.com>
StringRef
StringRef
StringRef
StringRef
StringRef
Ubuntu tests got stuck for 6 hours (but the macos ones passed). I'm closing + reopening this to rerun the tests. |
Still looks like they're hanging for you. I just posted about this on Discord, but I don't think the hang is due to your change? |
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.
LGTM, thanks for fixing this!
✅🟣 This contribution has been merged 🟣✅ Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours. We use Copybara to merge external contributions, click here to learn more. |
FYI I had to modify this internally to avoid the |
Landed in 0fc4272! Thank you for your contribution 🎉 |
[External] [stdlib] Delegate string comparisons to `StringRef` This is a follow-up to #2409. String comparisons for `StringRef` are implemented. `StringRef` make use of `memory.memcmp` for all of its 6 comparisons now, hopefully this change is ok. `String`'s and `StringLiteral`'s comparisons (`__eq__`, `__ne__`, `__lt__`, `__le__`, `__gt__`, & `__ge__`) are delegated to `StringRef`. As a result: 1. `String` comparisons uses its `_strref_dangerous`-method. And, 2. `StringLiteral`'s local `_memcmp`-function is removed (which was used "rather than memory.memcpy to avoid #31139 and #25100"). The base logic for all 18 comparisons are implemented in `StringRef`'s `__ne__` and `__lt__` methods. All other comparisons uses some variation/negation of either `__ne__` or `__lt__`. ORIGINAL_AUTHOR=Simon Hellsten <56205346+siitron@users.noreply.github.com> PUBLIC_PR_LINK=#2620 --------- Co-authored-by: Simon Hellsten <56205346+siitron@users.noreply.github.com> Closes #2620 MODULAR_ORIG_COMMIT_REV_ID: ed03ee44f06caed7cad77c9c909e83d3c2c2d3cc
@laszlokindrat Great that you got it to work! But, does this mean that |
Yes, unfortunately, although my understanding is that the code before your patch had a similar workaround. If you are interested, you can give it a try: the recursion probably comes from the constraint or some assert or print from the memcmp implemention. In theory, you might be able to refactor to work around this. |
Yes, the workaround is similar to what |
[External] [stdlib] Delegate string comparisons to `StringRef` This is a follow-up to modularml#2409. String comparisons for `StringRef` are implemented. `StringRef` make use of `memory.memcmp` for all of its 6 comparisons now, hopefully this change is ok. `String`'s and `StringLiteral`'s comparisons (`__eq__`, `__ne__`, `__lt__`, `__le__`, `__gt__`, & `__ge__`) are delegated to `StringRef`. As a result: 1. `String` comparisons uses its `_strref_dangerous`-method. And, 2. `StringLiteral`'s local `_memcmp`-function is removed (which was used "rather than memory.memcpy to avoid #31139 and #25100"). The base logic for all 18 comparisons are implemented in `StringRef`'s `__ne__` and `__lt__` methods. All other comparisons uses some variation/negation of either `__ne__` or `__lt__`. ORIGINAL_AUTHOR=Simon Hellsten <56205346+siitron@users.noreply.github.com> PUBLIC_PR_LINK=modularml#2620 --------- Co-authored-by: Simon Hellsten <56205346+siitron@users.noreply.github.com> Closes modularml#2620 MODULAR_ORIG_COMMIT_REV_ID: ed03ee44f06caed7cad77c9c909e83d3c2c2d3cc
[External] [stdlib] Delegate string comparisons to `StringRef` This is a follow-up to modularml#2409. String comparisons for `StringRef` are implemented. `StringRef` make use of `memory.memcmp` for all of its 6 comparisons now, hopefully this change is ok. `String`'s and `StringLiteral`'s comparisons (`__eq__`, `__ne__`, `__lt__`, `__le__`, `__gt__`, & `__ge__`) are delegated to `StringRef`. As a result: 1. `String` comparisons uses its `_strref_dangerous`-method. And, 2. `StringLiteral`'s local `_memcmp`-function is removed (which was used "rather than memory.memcpy to avoid #31139 and #25100"). The base logic for all 18 comparisons are implemented in `StringRef`'s `__ne__` and `__lt__` methods. All other comparisons uses some variation/negation of either `__ne__` or `__lt__`. ORIGINAL_AUTHOR=Simon Hellsten <56205346+siitron@users.noreply.github.com> PUBLIC_PR_LINK=modularml#2620 --------- Co-authored-by: Simon Hellsten <56205346+siitron@users.noreply.github.com> Closes modularml#2620 MODULAR_ORIG_COMMIT_REV_ID: ed03ee44f06caed7cad77c9c909e83d3c2c2d3cc
This is a follow-up to #2409.
String comparisons for
StringRef
are implemented.StringRef
make use ofmemory.memcmp
for all of its 6 comparisons now, hopefully this change is ok.String
's andStringLiteral
's comparisons (__eq__
,__ne__
,__lt__
,__le__
,__gt__
, &__ge__
) are delegated toStringRef
. As a result:String
comparisons uses its_strref_dangerous
-method. And,StringLiteral
's local_memcmp
-function is removed (which was used "rather than memory.memcpy to avoid #31139 and #25100").The base logic for all 18 comparisons are implemented in
StringRef
's__ne__
and__lt__
methods. All other comparisons uses some variation/negation of either__ne__
or__lt__
.