chore: unslice slice expressions #2982
Conversation
estensen
requested review from
rach-id,
evan-forbes,
rootulp and
cmwaters
as code owners
WalkthroughWalkthroughThe recent modifications involve streamlining slice operations in Go. Specifically, the redundant slice expressions have been eliminated when copying and comparing byte slices. This simplification impacts how elements are copied between slices and how byte slices are compared, ensuring a more concise and potentially more efficient implementation. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit's AI:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Thanks for your interest in improving our codebase! When I run gocritic, I see a few other issues. Is there any reason in particular you choose to focus on the unslice ones?
Also, if we want to avoid regressions, I propose we add gocritic to the list of linters in .golanci.yml.
$ golangci-lint run -E gocritic
pkg/da/data_availability_header.go:213:3: assignOp: replace `result = result << 1` with `result <<= 1` (gocritic)
result = result << 1
^
pkg/inclusion/commitment.go:43:3: assignOp: replace `cursor = cursor + treeSize` with `cursor += treeSize` (gocritic)
cursor = cursor + treeSize
^
pkg/inclusion/commitment.go:102:4: assignOp: replace `totalSize = totalSize - maxTreeSize` with `totalSize -= maxTreeSize` (gocritic)
totalSize = totalSize - maxTreeSize
^
pkg/inclusion/commitment.go:109:4: assignOp: replace `totalSize = totalSize - treeSize` with `totalSize -= treeSize` (gocritic)
totalSize = totalSize - treeSize
^
pkg/inclusion/paths.go:166:4: assignOp: replace `leafCursor = leafCursor + nodeRangeCursor` with `leafCursor += nodeRangeCursor` (gocritic)
leafCursor = leafCursor + nodeRangeCursor
^
pkg/inclusion/paths.go:167:4: assignOp: replace `nodeRangeCursor = nodeRangeCursor * 2` with `nodeRangeCursor *= 2` (gocritic)
nodeRangeCursor = nodeRangeCursor * 2
^
pkg/inclusion/nmt_caching_test.go:147:3: assignOp: replace `subLeafRange = subLeafRange / 2` with `subLeafRange /= 2` (gocritic)
subLeafRange = subLeafRange / 2
^
pkg/inclusion/paths_test.go:477:4: assignOp: replace `s = s + "l"` with `s += "l"` (gocritic)
s = s + "l"
^
pkg/inclusion/paths_test.go:479:4: assignOp: replace `s = s + "r"` with `s += "r"` (gocritic)
s = s + "r"
^
pkg/shares/powers_of_two.go:13:3: assignOp: replace `result = result << 1` with `result <<= 1` (gocritic)
result = result << 1
^
pkg/shares/share_builder.go:178:2: assignOp: replace `b.rawShareData[infoByteIndex] = b.rawShareData[infoByteIndex] ^ 0x01` with `b.rawShareData[infoByteIndex] ^= 0x01` (gocritic)
b.rawShareData[infoByteIndex] = b.rawShareData[infoByteIndex] ^ 0x01
^
pkg/shares/shares_test.go:21:16: appendAssign: append result not assigned to the same slice (gocritic)
firstShare := append(sparseNamespaceID,
^
pkg/shares/shares_test.go:27:32: appendAssign: append result not assigned to the same slice (gocritic)
firstShareWithLongSequence := append(sparseNamespaceID,
^
pkg/shares/shares_test.go:32:23: appendAssign: append result not assigned to the same slice (gocritic)
continuationShare := append(sparseNamespaceID,
^
x/blobstream/types/validator.go:98:23: unslice: could simplify []byte(e.Hex())[:] to []byte(e.Hex()) (gocritic)
return bytes.Compare([]byte(e.Hex())[:], []byte(o.Hex())[:]) == -1
^
pkg/square/square.go:265:7: unslice: could simplify square[:] to square (gocritic)
copy(square[:], txShares)
^
x/upgrade/keeper.go:33:2: singleCaseSwitch: found switch with default case only (gocritic)
switch version {
^
x/blob/client/testutil/integration_test.go:177:5: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
switch e.Type {
^
|
@rootulp I'm more than happy to add Also, it's up to you if you agree with all the issues the linter found. |
|
Yea I generally agree, smaller PRs are easier to review. However, in this case there's nothing stopping a subsequent PR from reverting the modifications you make in this PR. IMO if we're going to enable the gocritic linter (which I'm not strongly opinionated on for/against) then I'd rather we enable it in the config so that it runs in CI and subsequent PRs can't introduce any regressions. Does that make sense? Before we proceed with this line of work, it may be worthwhile to create a GH issue with a short description on gocritic so that we can get alignment from the team on whether it's something we should enable. Our team is generally in favor of linters that reduce the # of nits on PRs and enforce more codebase consistency so gocritic seems like something we should consider. |
|
Got it! Created #2987 |
|
Converted this to draft b/c if possible, I think it's preferable to fix all the go-critic violations in the same PR that enables the linter in golangci.yml. |
|
Closing because stale. We have another PR that does something similar: #3000 and enables the linter in golangci.yml so thanks for the idea and creating the issue! |
Found with gocritic ran with
golangci-lint run -E gocritic