Skip to content
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

feat: Migrate to math/rand/v2 #553

Merged
merged 5 commits into from
Jun 16, 2024
Merged

Conversation

SoulPancake
Copy link
Contributor

@SoulPancake
Copy link
Contributor Author

Please have a look @rueian ^_^

return
}

func BinaryString() []byte {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On self-review, we could probably rename this
Open to suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Maybe RandomBytes.

rand.Shuffle(n, swap)
}

func IntN(n int) int {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until.IntN looks vague. I think we can just replace it with the below FastRand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that, but for go !1.22
The regular usage of Intn would also be replaced with rngPool implementation, Should we do that? @rueian

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is okay.

@SoulPancake
Copy link
Contributor Author

Addressed @rueian Pls take a look

Copy link
Collaborator

@rueian rueian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Almost done. We just need to group and sort imports.

om/json.go Outdated
@@ -3,6 +3,7 @@ package om
import (
"context"
"encoding/json"
"github.com/oklog/ulid/v2"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should sort imports.

rueidis.go Outdated
@@ -7,8 +7,8 @@ import (
"context"
"crypto/tls"
"errors"
"github.com/redis/rueidis/internal/util"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should sort imports.

"errors"
"math/rand"
"github.com/redis/rueidis/internal/util"
"strconv"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should sort imports.

sentinel.go Outdated
@@ -5,15 +5,14 @@ import (
"context"
"errors"
"fmt"
"github.com/redis/rueidis/internal/util"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should sort imports.

@SoulPancake
Copy link
Contributor Author

@rueian Done, I guess this sorting should be okay

@rueian
Copy link
Collaborator

rueian commented Jun 3, 2024

@rueian Done, I guess this sorting should be okay

Yes, that is okay now. We just need to wait for #554.

@rueian
Copy link
Collaborator

rueian commented Jun 12, 2024

@rueian Done, I guess this sorting should be okay

Yes, that is okay now. We just need to wait for #554.

Hi @SoulPancake, #554 has been merged. Could you rebase this PR?

@SoulPancake
Copy link
Contributor Author

Done @rueian

@SoulPancake
Copy link
Contributor Author

@rueian Is this ready to be merged?

@rueian
Copy link
Collaborator

rueian commented Jun 14, 2024

Yes, this will be merged this weekend.

@rueian rueian merged commit ac9ce95 into redis:main Jun 16, 2024
21 checks passed
@rueian rueian mentioned this pull request Jun 16, 2024
@rueian
Copy link
Collaborator

rueian commented Jun 16, 2024

Thanks @SoulPancake!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants