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

Fix go vet failures #95

Merged
merged 2 commits into from
Aug 14, 2018
Merged

Fix go vet failures #95

merged 2 commits into from
Aug 14, 2018

Conversation

misha-ridge
Copy link
Contributor

No description provided.

Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @misha-tectonic.

LGTM. I ran go vet a while ago and was wondering about the (commented) unreachable code too. It would be good to know what was the intention (if any).

@liamsi liamsi requested a review from jaekwon August 14, 2018 13:12
@liamsi liamsi changed the base branch from master to develop August 14, 2018 13:14
@liamsi
Copy link
Contributor

liamsi commented Aug 14, 2018

Just out of curiosity: which version of golang / go vet did you use? I do rememeber that go vet complained about these lines: https://github.com/tendermint/iavl/pull/95/files#diff-3f1ad41879763caf285707f64678af17L74

But it disappeared with upgrading golang (to 1.10.3 on darwin).

@misha-ridge
Copy link
Contributor Author

Go 1.11beta2, beta3, rc1.

go vet test modules recursively, and go test runs go vet in 1.11, so any module that explicitly or implicitly includes iavl fails in the testsuite.

@liamsi
Copy link
Contributor

liamsi commented Aug 14, 2018

Thanks!

@liamsi
Copy link
Contributor

liamsi commented Aug 14, 2018

Interestingly, only c917ed2 seems necessary to make go vet happy:

go1.11rc1 test
# github.com/tendermint/iavl
./proof_range.go:368:6: lastDepth declared but not used
vet: typecheck failures
FAIL    github.com/tendermint/iavl [build failed]

After applying c917ed2 this passes.
In any case the unreachable code doesn't make sense. If there is an overflow append will panic and the second return is still unreachable.

@liamsi liamsi merged commit 5ee244d into cosmos:develop Aug 14, 2018
@misha-ridge
Copy link
Contributor Author

go test runs a subset of go vet tests. Running go vet standalone will emit the error about unreachable statement.

@liamsi
Copy link
Contributor

liamsi commented Aug 14, 2018

I also ran go1.11rc1 vet and it only complained about:

go1.11rc1 vet
# github.com/tendermint/iavl
./proof_range.go:368:6: lastDepth declared but not used
vet: typecheck failures

I was surprised about this too. I do remember vet complaining about the unreachable statement (a while ago though).

@misha-ridge
Copy link
Contributor Author

Oh, I see. go vet stops early: once you remove unused variables it starts complaining about unreachable code:

% git describe --tags
v0.9.2-1-gc917ed2
% go vet
# github.com/tendermint/iavl
./util.go:75: unreachable code

ridenaio pushed a commit to idena-network/iavl that referenced this pull request Jul 1, 2019
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.

2 participants