Skip to content

eth/fetcher: Remove unnecessary error check to satisfy static analysis #23739

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

Merged
merged 2 commits into from
Oct 18, 2021

Conversation

prestonvanloon
Copy link
Contributor

Fixes #23738. See #23738 for more context on the nilness analysis check issue.

@@ -277,29 +277,27 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool)
)
errs := f.addTxs(txs)
for i, err := range errs {
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if err != nil was bothering the nilness analysis check and is safe to remove.

Copy link
Contributor

@jwasinger jwasinger left a comment

Choose a reason for hiding this comment

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

LGTM. Change doesn't alter the code logic here. Although tbh, I'm slightly confused about the issue this claims to solve. The err can definitely be non-nil. So I'm wondering if maybe the nilness analysis check is complaining erroneously here.

@holiman
Copy link
Contributor

holiman commented Oct 18, 2021

Sorry, I didn't notice that you had made this PR, so I made one myself too

@holiman
Copy link
Contributor

holiman commented Oct 18, 2021

While we're fixing the code, it would be good to remove explicit error checks, and use errors.Is instead. Otherwise LGTM

@prestonvanloon
Copy link
Contributor Author

While we're fixing the code, it would be good to remove explicit error checks, and use errors.Is instead. Otherwise LGTM

@holiman I've done this now. Please take a look!

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

@holiman holiman added this to the 1.10.11 milestone Oct 18, 2021
@holiman holiman merged commit b97f578 into ethereum:master Oct 18, 2021
@prestonvanloon prestonvanloon deleted the fix-nilness-check branch October 18, 2021 21:29
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Oct 20, 2021
* eth/fetcher: fix nilness check ethereum#23738

* eth/fetcher: Use errors.Is. PR feedback from @holiman.
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
* eth/fetcher: fix nilness check ethereum#23738

* eth/fetcher: Use errors.Is. PR feedback from @holiman.
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.

eth/fetcher fails nilness static analysis check
3 participants