Skip to content

Remove deprecated StreamReader#1144

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
Kixunil:remove-stream-reader
Aug 1, 2022
Merged

Remove deprecated StreamReader#1144
apoelstra merged 1 commit intorust-bitcoin:masterfrom
Kixunil:remove-stream-reader

Conversation

@Kixunil
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil commented Jul 28, 2022

StreamReader was deprecated for a while, yet we needlessly maintain it
and it eats our testing time. This change removes it completely.

Closes #1123

@Kixunil Kixunil added API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...) labels Jul 28, 2022
@Kixunil Kixunil force-pushed the remove-stream-reader branch 3 times, most recently from ac65947 to 8d7f4cb Compare July 30, 2022 12:49
`StreamReader` was deprecated for a while, yet we needlessly maintain it
and it eats our testing time. This change removes it completely.

Closes rust-bitcoin#1123
@Kixunil Kixunil force-pushed the remove-stream-reader branch from 8d7f4cb to 0c9c141 Compare July 30, 2022 12:59
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 0c9c141 (with the function rename).

let mut reader = StreamReader::new(&big_block[..], None);
let block: Block = reader.read_next().unwrap();
let mut reader = &big_block[..];
let block = Block::consensus_decode(&mut reader).unwrap();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't groked the bench mark code in general but this function probably should be renamed to say what it is bench marking, possibly bench_block_deserialize_logic?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I didn't read the function name. 😅 I will have to look if it's even needed, maybe it should be removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cheers, I could not work out if it was adding value or not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My intuition is we should probably drop it. Can do this in a followup PR

@Kixunil Kixunil mentioned this pull request Aug 1, 2022
3 tasks
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utACK 0c9c141.

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 0c9c141

@apoelstra apoelstra merged commit ef5014f into rust-bitcoin:master Aug 1, 2022
@Kixunil Kixunil deleted the remove-stream-reader branch August 1, 2022 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Useless TCP test

4 participants