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

#[inline]ing adjustments and minor syntax/formatting changes #2016

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

marc0246
Copy link
Contributor

@marc0246 marc0246 commented Oct 3, 2022

Added the #[inline] attribute to functions that could use it, and removed it from functions where it could do harm, or wasn't doing anything. The attribute is only useful to allow for the compiler to inline functions across a crate boundry.

For reference:

  • Putting #[inline] on methods of error types is always a bad idea, as these are supposed to be exceptional states. In this case it could actually do harm, as it would increase code size for paths that are almost never taken and slightly increase compile times (because the compiler won't be able to compile our crate and their crate in parallel, if they make use of the methods).
  • I removed it from impl Debugs with similar reasoning, but this can do no harm since you don't need to debug-print in production (I hope).
  • Generic methods and non-generic methods on generic types never need the attribute, because the methods need to be instantiated with a concrete type at the use-site, so they always need to cross the crate boundry and are always inlinable for rustc.
  • Private methods never need the attribute as they can't cross the crate boundry.

I also made some syntactical changes here and there, mainly I removed needless dereferencing followed by ref-matching. For this I also added the Copy trait for all SPIR-V types, so we don't need to deal with a bunch of referencing and dereferencing. Kind of like how all ash types are Copy.

Other than that, I took the liberty to do some minor changes to formatting as I was looking through the codebase, hopefully you don't mind. Mainly I made sure that lines don't exceed the rustfmt limit of 100 columns, and that doc comments use correct formatting.

All in all the intention was to hopefully make the code a bit cleaner. It's all in one humongous commit because I didn't have anything specific in mind as I was going through it all other than the inlining, so my apologies. Hopefully it's not too big of a hassle to review.

@Rua
Copy link
Contributor

Rua commented Oct 4, 2022

Why did you remove #[inline] from trait methods too?

@marc0246
Copy link
Contributor Author

marc0246 commented Oct 4, 2022

Which ones?

@Rua
Copy link
Contributor

Rua commented Oct 4, 2022

For example on BufferContents, BufferViewAbstract, PrimaryCommandBuffer, SecondaryAutoCommandBuffer and many others.

@marc0246
Copy link
Contributor Author

marc0246 commented Oct 4, 2022

Because those traits were implemented on generic types, which need to be instanciated at the place they are used, so those methods don't exist until then. They always are duplicated and always cross the crate boundry, so those attributes weren't doing anything.

@Rua
Copy link
Contributor

Rua commented Oct 4, 2022

I see, thanks.

@Rua Rua merged commit b8d7cc3 into vulkano-rs:master Oct 4, 2022
@marc0246 marc0246 deleted the inlining branch October 4, 2022 08:51
marc0246 added a commit to marc0246/vulkano that referenced this pull request Oct 4, 2022
Rua pushed a commit that referenced this pull request Oct 4, 2022
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