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

generator: Apply must_use attributes to all Vulkan structs #845

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

MarijnS95
Copy link
Collaborator

@MarijnS95 MarijnS95 commented Dec 4, 2023

All structs are marked as Copy, and builders move self for a convenient builder pattern directly on default() (using &mut requires first keeping the instance alive in a let binding). This however leads to builder functions accidentally moving a copy of self, mutating it, and dropping the result directly when the caller did not consume the returned value in ad-hoc field updates, in turn leaving the author confused why their code compiles without warnings while the struct was not updated.

Annotating all Vulkan structs with #[must_use] should at least give them an idea why, especially because of Rust allowing us to set a custom message.

@Ralith
Copy link
Collaborator

Ralith commented Dec 4, 2023

using &mut requires first storing the builder on the heap

I think you mean stack, but yeah.

generator/src/lib.rs Show resolved Hide resolved
@MarijnS95
Copy link
Collaborator Author

using &mut requires first storing the builder on the heap

I think you mean stack, but yeah.

Yup, that's most common, though either works when it can be mutably borrowed while having a longer lifetime.

@MarijnS95 MarijnS95 changed the title generator: Apply must_use attributes to all struct setter functions generator: Apply must_use attributes to all Vulkan structs Dec 5, 2023
@MarijnS95
Copy link
Collaborator Author

Reworded the commit message to mention let binding :)

@MarijnS95 MarijnS95 requested a review from Ralith December 5, 2023 19:41
generator/src/lib.rs Outdated Show resolved Hide resolved
All structs are marked as `Copy`, and builders move `self` for a
convenient builder pattern directly on `default()` (using `&mut`
requires requires first keeping the instance alive in a `let` binding).
This however leads to builder functions accidentally moving a copy of
`self`, mutating it, and dropping the result directly when the caller
did not consume the returned value in ad-hoc field updates, in turn
leaving the author confused why their code compiles without warnings
while the struct was not updated.

Annotating all Vulkan structs with `#[must_use]` should at least give
them an idea why.
@MarijnS95 MarijnS95 merged commit e6d80ba into master Dec 5, 2023
20 checks passed
@MarijnS95 MarijnS95 deleted the builder-must_use branch December 5, 2023 21:09
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