[stdlib] String comparisons implemented

[siitron]

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.

[VMois]

Are there any reasons to close a PR? This looks like quite a nice implementation.

[siitron]

@VMois Thank you! I closed this PR before because I couldn't pass the tests (because I didn't know that the math package wasn't supported to use because of cyclic dependencies). But now I've replaced min with if/else-statements and that nicely resulted in fewer comparisons.

I can't complete ./stdlib/scripts/run-tests.sh locally due to some issues I have with WSL, but the current version should be fine now.

[abduld]

Suggestion: you can consolidate the duplication between this and the implementation in String by implementing the comparison on StringRef

[siitron]

Do you mean something like return StringRef(self) < StringRef(rhs) as __lt__ for StringLiteral (and return self._strref_dangerous() < rhs._strref_dangerous() for String), etc.?

I didn't even consider StringRef (forgot it existed), and I tried to match __lt__ with their respective __eq__-methods. Right now String, StringLiteral, and StringRef does __eq__ in three separate ways, so I thought there was some reason for keeping them apart ATM(?). Would it still be safe to use memory.memcmp in the StringRef __lt__ implementation (considering it currently looks like StringLiteral has to use it's own local _memcmp, and StringRef's __eq__ also avoids it), or should StringRef's __lt__ be done like its __eq__?

What do you think of:

    @always_inline("nodebug")
    fn __lt__(self, rhs: StringRef) -> Bool:
        """Compare this StringRef to the RHS using LT comparison.

        Args:
            rhs: The other StringRef to compare against.

        Returns:
            True if this StringRef is strictly less than the RHS StringRef and False otherwise.
        """
        var len1 = len(self)
        var len2 = len(rhs)

        if len1 < len2:
            return memcmp(self.data, rhs.data, len1) <= 0
        else:
            return memcmp(self.data, rhs.data, len2) < 0

[laszlokindrat]

This is great, thank you! Do you think you could do a followup where you move the core logic to StringRef and delegate to that from both String and StringLiteral?

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

[siitron]

This is great, thank you! Do you think you could do a followup where you move the core logic to StringRef and delegate to that from both String and StringLiteral?

Sure np, I'll create a new PR for the StringRef stuff!

Some thoughts (that I'll probably repeat in the PR):

@laszlokindrat Does this mean that StringLiteral no longer needs its private _memcmp-function (assuming __eq__ and __ne__ are also delegated to StringRef), i.e. StringLiteral (and StringRef) are ok to use memory.memcmp now? If yes, then I can make the 18 comparison methods all just use __eq__ and __lt__ in StringRef. I'm asking since StringLiteral's private _memcmp function has this comment:

# Use a local memcmp rather than memory.memcpy to avoid #31139 and #25100.

(I can't access "#31139 and #25100", I'm guessing its some internal stuff).

Also, for String to be delegated to StringRef I'd have to use String's _strref_dangerous-method which I was a bit hesitant to do

[JoeLoser]

Landed in today's nightly: #2615. Thanks for the contribution!

[laszlokindrat]

@laszlokindrat Does this mean that StringLiteral no longer needs its private _memcmp-function (assuming __eq__ and __ne__ are also delegated to StringRef), i.e. StringLiteral (and StringRef) are ok to use memory.memcmp now? If yes, then I can make the 18 comparison methods all just use __eq__ and __lt__ in StringRef. I'm asking since StringLiteral's private _memcmp function has this comment:

Use a local memcmp rather than memory.memcpy to avoid #31139 and #25100.

(I can't access "#31139 and #25100", I'm guessing its some internal stuff).

Good question. If you don't mind, would you just give it a go, and let's see what breaks? If things don't work, I will take a look at those tickets.

Also, for String to be delegated to StringRef I'd have to use String's _strref_dangerous-method which I was a bit hesitant to do

I think this should be okay, since the lifetimes of self should be guaranteed to outlive method calls, so you can take a dangerous reference in these method implementations.

[siitron]

If you don't mind, would you just give it a go, and let's see what breaks? If things don't work, I will take a look at those tickets.

@laszlokindrat You can take a look at #2620 if you'd like :)