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

primitives: Inline public functions #121

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

tcharding
Copy link
Member

Audit the primitives module and all submodules; add inline to all public methods that are small enough (excl. error impls).

Audit the `primitives` module and all submodules; add inline to all
public methods that are small enough (excl. error impls).
@apoelstra
Copy link
Member

concept ACK. I'm not sure what your exact criteria is and I'm not sure how to vet your choices, but nothing here seems particularly egregious.

If we wanted a specific policy I think "one-liners and functions we've actually benchmarked to be faster with inlining" would be a good one.

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 847e1c9

@tcharding
Copy link
Member Author

tcharding commented Aug 10, 2023

I'm not sure what your exact criteria is

My critera was, after re-reading #1595 discussion, "kind of guess at what the assembler would be, given I know very little about compiler optimisations, and then aggressively inline. And if I was really unsure look at stdlib". So I skipped all Display things. I skipped all error code, and I inlined most other things, IIRC only one iterator next seemed to big to inline, most everything else got done.

Copy link
Member

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

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

ACK 847e1c9

@apoelstra apoelstra merged commit 72e7dd8 into rust-bitcoin:master Aug 15, 2023
12 checks passed
@apoelstra apoelstra deleted the 08-10-inline branch August 15, 2023 16:44
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.

3 participants