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

check for rand.Int error added via latest PR #224

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cppforlife
Copy link
Contributor

No description provided.

plus minor stylistic tweaks

Signed-off-by: Dmitriy Kalinin <[email protected]>
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

The PR looks good I just have a minor comment that maybe we can improve.

}
return string(s[n.Int64()]), nil
}

func localCryptoShuffle(n int, swap func(i, j int)) {
func (*PasswordReconciler) localCryptoShuffle(n int, swap func(i, j int)) error {
if n < 0 {
panic("invalid argument to Shuffle")
Copy link
Member

Choose a reason for hiding this comment

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

This should not panic the generator unless it is an internal inconsistency error. This looks like invalid input from the spec itself, since we now have an output error maybe we can return it instead.

@neil-hickey
Copy link
Contributor

hey @joaopapereira / @cppforlife, just checking in to see if we can push this PR through? Any blockers or discussions needed?

@neil-hickey
Copy link
Contributor

Hey @cppforlife / @joaopapereira . This one has been sitting for a long time, shall we close it out? Or can we get this over the line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

3 participants