Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Add Hash::from_inner and prevent double allocation in FromHex using it #40

Merged
merged 2 commits into from
May 1, 2019

Conversation

stevenroose
Copy link
Collaborator

No description provided.

src/hex.rs Outdated
fn next_back(&mut self) -> Option<Result<u8, Error>> {
let current_len = self.sl.len();
if current_len % 2 == 1 {
Some(Err(Error::OddLengthString(self.sl.len())))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not use current_len here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty to fix it in a separate commit, do you think I should squash it into Andrew's commit?

let current_len = self.sl.len();
if current_len % 2 == 1 {
Some(Err(Error::OddLengthString(self.sl.len())))
} else if self.sl.is_empty() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not use current_len here as well? I would start with if current_len == 0 { ... } else if current_len % 2 == 1 { ... } personally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I see this is mirroring the impl of Iterator, which if fine I guess.

@stevenroose
Copy link
Collaborator Author

Can't approve my own PR. So ACK d71dd99

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

LGTM, please tell me if I should squash the fixup commit and reapprove.

impl<'a> DoubleEndedIterator for HexIterator<'a> {
fn next_back(&mut self) -> Option<Result<u8, Error>> {
let current_len = self.sl.len();
if current_len % 2 == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo this check shouldn't happen for every element, but I'll open a separate PR.

src/hex.rs Outdated
fn next_back(&mut self) -> Option<Result<u8, Error>> {
let current_len = self.sl.len();
if current_len % 2 == 1 {
Some(Err(Error::OddLengthString(self.sl.len())))
Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty to fix it in a separate commit, do you think I should squash it into Andrew's commit?

@apoelstra apoelstra merged commit 690c76a into rust-bitcoin:master May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants