-
Notifications
You must be signed in to change notification settings - Fork 20.7k
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
all: fix some go-critic linter warnings #23709
all: fix some go-critic linter warnings #23709
Conversation
./accounts/abi/reflect.go:126:3: dupBranchBody: both branches in if statement has same body It's not a bug, but it looks like it was an attempt to perform some optimization (?) for structs; when we know it's a struct, we would call `setStruct()` directly instead of going through an extra check in `set()`. Another option would be to remove the branching and use one code for both of them, the one that calls `set()` in all cases. Regexp changes inspired by this warning: ./signer/core/validation.go:24:45: badRegexp: suspicious char range `,-.` in [A-Za-z0-9!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~ ] Accidentally (?) this regexp works as intended, `[,-.]` is interpreted as a char range, `,` is 44, `.` is 46; so we get a range of 3 chars, [`,`, char(45), `.`], it happens that char(45) is `-`; but our intentions can be more clearly expressed by enumerating all 3 characters in a group without a sloppy range, like `[,\-.]` (note that `-` is escaped now). For `[]rune(t.Type)[0]` code we could use `utf8.DecodeRuneInString`; this avoids the need to clone `string` bytes into a fresh `[]rune` just to take the first rune. A couple of deprecation comments used the non-standard format. Go tools mostly respect this format: `Deprecated: <text>`, so I changed these comments accordingly. The `bw.Cmp(bw)` line in tests is clearly a copy/paste mistake quite common to the tests. Based on the code context, I'm assuming `bh` should be used in place of one of the operands. My personal favorites: ./cmd/faucet/faucet.go:744:32: regexpPattern: '.com' should probably be '\.com' ./cmd/faucet/faucet.go:870:32: regexpPattern: '.net' should probably be '\.net' Sometimes we forget to escape `.` when validation URLs. This leads to the peculiar effect that allows `google.net/` match `googlexnet/` which may or may not be a problem (depends on the entire regexp and the context). `go-critic` recommends to always escape `.` in things like `.com` and `.net`. A couple of simple code changes where we simplify the code by calling `WriteString(s)` instead of using `Write([]byte(s))`. For the benchmark changes, I found these issues: ./core/bloombits/generator_test.go:73:4: rangeExprCopy: copy of input (524288 bytes) can be avoided with &input ./core/bloombits/generator_test.go:92:4: rangeExprCopy: copy of input (524288 bytes) can be avoided with &input This is real and Go does copy the **array** completely, but in this case the loop body dominates the run time, so copying 0.5mb per benchmark iteration is not a big deal, but it adds some distortion to the benchmark results. After changing `input` to `&input` I got this diff: ``` name old time/op new time/op delta Generator/empty-8 1.00ms ± 2% 0.97ms ± 1% -3.58% (p=0.000 n=9+9) Generator/random-8 30.2ms ± 1% 30.0ms ± 2% ~ (p=0.065 n=9+10) ``` It saves ~0.03ms on my machine. It's not a `3.58%` speedup, it's a noise reduced by this amount. Which is good.
The failing test is the one that had self-comparison in |
@@ -21,7 +21,7 @@ import ( | |||
"regexp" | |||
) | |||
|
|||
var printable7BitAscii = regexp.MustCompile("^[A-Za-z0-9!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~ ]+$") | |||
var printable7BitAscii = regexp.MustCompile("^[A-Za-z0-9!\"#$%&'()*+,\\-./:;<=>?@[\\]^_`{|}~ ]+$") |
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 \\
is already there, later in the string, [\\]
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 string is not unescaped, \\
is \
, not \\
.
So I'm escaping the -
to avoid the weird char range; please read the commit message description about it. :)
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.
Doh, rtfm me :)
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.
Can we unescape this too one level and use the backtick operators as the outer delimiters? Seems like it would make things easier
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.
It is not possible to use backquote with this string because it contains the backquote character.
The test failed because it didn't use the correct state root to get the reference block state. Instead, it tried to compare reconstructed state for each block to the head state of the reference chain.
Co-authored-by: Martin Holst Swende <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
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.
LGTM
This doesn't fix all go-critic warnings, just the most serious ones. Co-authored-by: Felix Lange <[email protected]> Co-authored-by: Martin Holst Swende <[email protected]>
This doesn't fix all go-critic warnings, just the most serious ones. Co-authored-by: Felix Lange <[email protected]> Co-authored-by: Martin Holst Swende <[email protected]>
It's not a bug, but it looks like it was an attempt to perform some optimization (?)
for structs; when we know it's a struct, we would call
setStruct()
directly insteadof going through an extra check in
set()
.Another option would be to remove the branching and use one code for both of them,
the one that calls
set()
in all cases.Regexp changes inspired by this warning:
Accidentally (?) this regexp works as intended,
[,-.]
is interpreted as a char range,,
is 44,.
is 46; so we get a range of 3 chars, [,
, char(45),.
], it happensthat char(45) is
-
; but our intentions can be more clearly expressed by enumeratingall 3 characters in a group without a sloppy range, like
[,\-.]
(note that-
is escaped now).For
[]rune(t.Type)[0]
code we could useutf8.DecodeRuneInString
; this avoids theneed to clone
string
bytes into a fresh[]rune
just to take the first rune.A couple of deprecation comments used the non-standard format.
Go tools mostly respect this format:
Deprecated: <text>
, soI changed these comments accordingly.
The
bw.Cmp(bw)
line in tests is clearly a copy/paste mistake quitecommon to the tests. Based on the code context, I'm assuming
bh
shouldbe used in place of one of the operands.
My personal favorites:
Sometimes we forget to escape
.
when validation URLs.This leads to the peculiar effect that allows
google.net/
matchgooglexnet/
which may or may not be a problem (depends on the entire regexp and the context).
go-critic
recommends to always escape.
in things like.com
and.net
.A couple of simple code changes where we simplify the code by calling
WriteString(s)
instead of using
Write([]byte(s))
.For the benchmark changes, I found these issues:
This is real and Go does copy the array completely, but in this
case the loop body dominates the run time, so copying 0.5mb per benchmark
iteration is not a big deal, but it adds some distortion to the benchmark results.
After changing
input
to&input
I got this diff:It saves ~0.03ms on my machine. It's not a
3.58%
speedup, it's anoise reduced by this amount. Which is good.