[stdlib] Delegate string comparisons to StringRef

[siitron]

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__.

[siitron]

Ubuntu tests got stuck for 6 hours (but the macos ones passed). I'm closing + reopening this to rerun the tests.

[JoeLoser]

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?

[laszlokindrat]

LGTM, thanks for fixing this!

[modularbot]

✅🟣 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.

[laszlokindrat]

FYI I had to modify this internally to avoid the memcmp recursion. It's the same problem that necessitated the _memcmp workaround, and I did a similar workaround as well.

[modularbot]

Landed in 0fc4272! Thank you for your contribution 🎉

[siitron]

FYI I had to modify this internally to avoid the memcmp recursion. It's the same problem that necessitated the _memcmp workaround, and I did a similar workaround as well.

@laszlokindrat Great that you got it to work! But, does this mean that String now have to use a non-vectorized version of memcmp when doing comparisons?

[laszlokindrat]

@laszlokindrat Great that you got it to work! But, does this mean that String now have to use a non-vectorized version of memcmp when doing comparisons?

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.

[siitron]

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 StringLiteral had before, but String can use the regular memcmp. I could maybe try and see if anything could be done about the recursion but idk, I don't even know how to trigger it. It's at least nice to have it all in one place :)

[siitron]

Weird, the tests seem to pass on my fork: siitron#1 🤔

I'm creating a new PR if you want to take a look: #2740