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

Commits on Oct 11, 2021

  1. all: fix some go-critic linter warnings

         ./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 committed Oct 11, 2021
    Copy the full SHA
    9c22bbf View commit details
    Browse the repository at this point in the history

Commits on Oct 13, 2021

  1. eth/protocols/eth: fix GetNodeData test

    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.
    fjl committed Oct 13, 2021
    Copy the full SHA
    d8d0b4f View commit details
    Browse the repository at this point in the history
  2. Update cmd/faucet/faucet.go

    Co-authored-by: Martin Holst Swende <martin@swende.se>
    fjl and holiman committed Oct 13, 2021
    Copy the full SHA
    af8aa02 View commit details
    Browse the repository at this point in the history
  3. Update cmd/faucet/faucet.go

    Co-authored-by: Martin Holst Swende <martin@swende.se>
    fjl and holiman committed Oct 13, 2021
    Copy the full SHA
    17a9fb0 View commit details
    Browse the repository at this point in the history
  4. accounts/abi: simplify setSlice

    fjl committed Oct 13, 2021
    Copy the full SHA
    e0f8864 View commit details
    Browse the repository at this point in the history