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

refactor: Drop error results from functions that never fail #777

Merged
merged 7 commits into from
May 30, 2023

Conversation

elias-orijtech
Copy link
Contributor

@elias-orijtech elias-orijtech commented May 24, 2023

The hashing procedure can never fail, and not returning an error frees up callers from worrying about it.

Also includes a performance optimization.

Fixes #757.
Fixes #756.

CC @odeke-em

There are no cases where Hash would fail, so don't return an error
value. Simplifies every caller, as can be seen from the test changes.

Fixes cosmos#757
The byte buffer in Node.hashWithCount is not necessary because the
hash.Hash interface implements io.Writer. Use that directly instead to
save allocations and memory.
There's no need because the hashing can never fail.
@elias-orijtech elias-orijtech requested a review from a team as a code owner May 24, 2023 20:40
@elias-orijtech elias-orijtech changed the title refactor: Drop error results from Node._hash and ImmutableTree.hash refactor: Drop error results from functions that never fail May 24, 2023
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

UtAck

@tac0turtle
Copy link
Member

tac0turtle commented May 25, 2023

could we get a changelog for the api break also linting is failing

Tree construction never fails, so don't return an error.

Fixes cosmos#756
@elias-orijtech
Copy link
Contributor Author

could we get a changelog for the api break also linting is failing

Done.

@tac0turtle tac0turtle enabled auto-merge (squash) May 26, 2023 09:44
@tac0turtle
Copy link
Member

could we fix the linting issues as well please

@tac0turtle tac0turtle merged commit a213e88 into cosmos:master May 30, 2023
6 of 8 checks passed
elias-orijtech added a commit to elias-orijtech/iavl that referenced this pull request May 30, 2023
elias-orijtech added a commit to elias-orijtech/iavl that referenced this pull request May 30, 2023
Found by the linter in cosmos#777.

Remove an unused linter directive while here.
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.

Drop error result from Node.hashWithCount Drop error result from NewMutableTree/NewMutableTreeWithOpts
2 participants