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

Jump to bottom

Avoid allocation in build_scriptint #1033

Merged
merged 1 commit into from Jun 3, 2022
Merged

Avoid allocation in build_scriptint #1033

merged 1 commit into from Jun 3, 2022

Conversation

stevenroose
Copy link
Collaborator

@stevenroose stevenroose commented Jun 2, 2022 •
edited by apoelstra

Hehe, reason for party, let's invite apoelstra !

@apoelstra
Copy link
Member

I like this approach -- but I think there's a more elegant way to do this, where write_scriptint takes an arbitrary W: Write, doesn't do its own length-tracking, and the caller gives it a Cursor<&mut [u8]> to get the length-tracking.

apoelstra
apoelstra approved these changes Jun 2, 2022
View reviewed changes
Copy link
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 c80dbc2

acking this version, as not to let the perfect be the enemy of the good. It's great to eliminate an allocation!

tcharding
tcharding approved these changes Jun 3, 2022
View reviewed changes
Copy link
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 c80dbc2

src/blockdata/script.rs
Comment on lines -1109 to +1117
use super::build_scriptint;
use super::write_scriptint;
Copy link
Member

Choose a reason for hiding this comment

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

We can just delete these lines, wildcard import right above :) Probably not worth requiring re-acks just to do this though.

sanket1729
sanket1729 approved these changes Jun 3, 2022
View reviewed changes
Copy link
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.

Code review ACK

@apoelstra apoelstra merged commit de17554 into rust-bitcoin:master Jun 3, 2022
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1033: Avoid allocation in build_scrip…
f465b79 
…tint

c80dbc2 Avoid allocation in build_scriptint (Steven Roose)

Pull request description:

  Hehe, reason for party, let's invite apoelstra !

ACKs for top commit:
  apoelstra:
    ACK c80dbc2
  tcharding:
    ACK c80dbc2

Tree-SHA512: 8446e765d8b9fa562f636817327db6fad4bb9c906d3f69fda76e61cd258fc4c296e6ffaa440a357125c2ab45603eb05c58cb8d6822deea2fe5746e5c7c3f1e4d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stevenroose @apoelstra @sanket1729 @tcharding