-
Notifications
You must be signed in to change notification settings - Fork 145
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
Updating linter version to v1.41 #237
Conversation
cd24567
to
7c97843
Compare
Was this change made by the linter or did you make it manually? |
I manually changed the instances reported by linter as insecure. |
7c97843
to
92a0ed0
Compare
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.
Just some minor readability things.
for i := 0; i < N; i++ { | ||
p[i] = int16(mathRand.Intn(18*int(Q) - 9*int(Q))) | ||
p[i] = int16(int32(r[i] % max)) |
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.
This change looks equivalent to me. However, the way the original code was written is weird. (Why 18 * int(Q) - 9 * int(Q)
instead of just 9 * int(Q)
?) This makes me think it might be worth double checking that this is the correct range.
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.
@bwesterb could you please give us some light here?
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.
let's move this change forward, and do changes in other PR if bwesterb thinks it's needed.
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.
The intended code was int16(mathRand.Intn(9*int(Q)) - mathRand.Intn(18*int(Q)))
.
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.
Thanks Bas, I will fix this on #240
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.
Approved, with a few suggestions.
c80bce5
to
0e53b81
Compare
Bumping up golangci-lint version to v1.41