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

all: fix some go-critic linter warnings #23709

Merged
merged 5 commits into from Oct 13, 2021
Merged

all: fix some go-critic linter warnings #23709

merged 5 commits into from Oct 13, 2021

Conversation

quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Oct 11, 2021

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

     ./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.
@quasilyte
Copy link
Contributor Author

The failing test is the one that had self-comparison in bw.Cmp(bw).
I would guess that it implies that some other code needs to be fixed.

@fjl fjl added this to the 1.10.10 milestone Oct 12, 2021
cmd/faucet/faucet.go Outdated Show resolved Hide resolved
cmd/faucet/faucet.go Outdated Show resolved Hide resolved
eth/protocols/eth/handler_test.go Show resolved Hide resolved
@@ -21,7 +21,7 @@ import (
"regexp"
)

var printable7BitAscii = regexp.MustCompile("^[A-Za-z0-9!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~ ]+$")
var printable7BitAscii = regexp.MustCompile("^[A-Za-z0-9!\"#$%&'()*+,\\-./:;<=>?@[\\]^_`{|}~ ]+$")
Copy link
Contributor

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, [\\]

Copy link
Contributor Author

@quasilyte quasilyte Oct 13, 2021

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh, rtfm me :)

Copy link
Contributor

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

Copy link
Contributor

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.

fjl and others added 3 commits October 13, 2021 14:27
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 <martin@swende.se>
Co-authored-by: Martin Holst Swende <martin@swende.se>
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@fjl fjl merged commit 778ff94 into ethereum:master Oct 13, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Oct 13, 2021
This doesn't fix all go-critic warnings, just the most serious ones.

Co-authored-by: Felix Lange <fjl@twurst.com>
Co-authored-by: Martin Holst Swende <martin@swende.se>
@quasilyte quasilyte deleted the quasilyte/fix_gocritic_warnings branch November 7, 2021 11:23
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
This doesn't fix all go-critic warnings, just the most serious ones.

Co-authored-by: Felix Lange <fjl@twurst.com>
Co-authored-by: Martin Holst Swende <martin@swende.se>
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.

None yet

3 participants