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: less #[inline(always)] #26

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

FreezyLemon
Copy link
Contributor

It's a bit unnecessary to have inline(always) on getter functions IMHO. The polyX functions are also very small, so a normal inline attribute is enough.

@shssoichiro
Copy link
Collaborator

shssoichiro commented Sep 24, 2024

Could you change the ones you removed to be normal #[inline] instead? I think the original intent of these was to allow them to be inlined across crates, since they're so small and Rust does not do that by default unless you enable LTO. I can't remember the reasoning for setting them to #[inline(always)], but it does indeed seem unnecessary.

@FreezyLemon
Copy link
Contributor Author

Sure. I didn't actually know Rust inlined across crates at all without LTO, it's good to know that #[inline] enables that

Copy link
Collaborator

@shssoichiro shssoichiro left a comment

Choose a reason for hiding this comment

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

Thanks!

@shssoichiro shssoichiro merged commit 9f07a7c into rust-av:main Sep 24, 2024
2 checks passed
@FreezyLemon FreezyLemon deleted the reduce-inline-always branch September 24, 2024 21:45
@FreezyLemon
Copy link
Contributor Author

I think the original intent of these was to allow them to be inlined across crates, since they're so small and Rust does not do that by default unless you enable LTO.

I stumbled over something just now and remembered this PR: This has been changed for 1.75, and is now done automatically for small enough functions. Not saying this should be changed here, but just FYI.

@shssoichiro
Copy link
Collaborator

Huh, I had forgotten about that change. Probably worth keeping this for now for users on older Rust versions, since it doesn't hurt to keep around.

I also remembered that the reason I put inline(always) on the poly functions is because I really did want them inlined always, but as you mentioned, a normal #[inline] is probably enough to hint that they should be inlined. It almost feels like it would be a compiler bug if it didn't.

@FreezyLemon
Copy link
Contributor Author

I also remembered that the reason I put inline(always) on the poly functions is because I really did want them inlined always

Yeah that's fair enough, inlining is critical for those. Maybe that should've been left as-is actually.

It almost feels like it would be a compiler bug if it didn't.

Also true. But it might've been better still to just force it. IDK

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.

2 participants