-
Notifications
You must be signed in to change notification settings - Fork 192
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
ash-window: Make enumerate_required_extensions
return &[*const c_char]
#590
Conversation
Currently, the mismatch between what is returned by let extensions: Vec<_> = extensions
.iter()
.map(|ext| ext.as_ptr())
.collect(); This PR will make that unnecessary. |
e324255
to
a66e39e
Compare
Note: I don't think I've touched any of the areas with clippy warnings. |
a66e39e
to
8aae071
Compare
(rebased to incorporate #591 ... and fixed a relevant clippy warning now that the flood of others is gone) |
8aae071
to
bab93ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've been wanting this for ages!
It should be noted that this will require ash-window's ash dependency to be collapsed down to the latest version once released. |
bab93ed
to
6c42083
Compare
(rebasing onto latest commit) |
Just holding off on merging until @MarijnS95 has a look since it's breaking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been hesitant to do this in the past because of not having lifetime annotations on *const c_char
, but these are anyway all 'static
and there's no owned Vec<>
being returned anymore (separate, still valid change) which users could accidentally append their own pointers with different lifetime to (this is a problem I vaguely recall having in our own application surrounding extension names, but not sure how since these would have anyway been static
string literals 🤔 ).
As such, this change seems totally fine and sensible to me.
Can we make fn
const
as a non-breaking patch release?
This PR makes the names const in a separate commit from the breaking change specifically to allow that. |
@swooster We're still squash-merging here, so that'd have little effect unless the commit is transplanted to a separate PR (or #594 is reopened, retitled, and merged). There are currently anyway very few changes (both on |
enumerate_required_extensions
return &[*const c_char]
This alters enumerate_required_extensions() to return the same type that is expected by vk::InstanceCreateInfoBuilder::enabled_extension_names(), allowing simple Vulkan apps to omit the boilerplate of mapping to an intermediate Vec<*const c_char>.
6c42083
to
6af55b8
Compare
Rebased to regenerate against |
@swooster Are you aware that your email address used in the commits isn't associated with your GitHub account? |
If the only difference between the two is that my github account is I've gone ahead and added |
@swooster it seems to be your initials + surname, separated with dots, at gmail dot com. See the EDIT: It looks like you added that address to your GH account as your name in those commits is now clickable and links back to your profile. |
[ash-rs#590] introduced an unsuspecting MSRV bump. While we're pro-ba-bly fine having these at the benefit of better code (in this case more appropriate `const` annotations), they should at least be clear to us when merging through a CI failure (or up-front bump of this version in the CI script). At the same time setting [`rust-version` in `Cargo.toml`] provides a more helpful "requires newer rustc" error message (since Rust 1.56.0) instead of showing potentially tons of irrelevant compile errors in this crate to the user. [ash-rs#590]: ash-rs#590 [`rust-version` in `Cargo.toml`]: https://doc.rust-lang.org/cargo/reference/manifest.html?highlight=pack#the-rust-version-field
) [#590] introduced an unsuspecting MSRV bump. While we're pro-ba-bly fine having these at the benefit of better code (in this case more appropriate `const` annotations), they should at least be clear to us when merging through a CI failure (or up-front bump of this version in the CI script). At the same time setting [`rust-version` in `Cargo.toml`] provides a more helpful "requires newer rustc" error message (since Rust 1.56.0) instead of showing potentially tons of irrelevant compile errors in this crate to the user. [#590]: #590 [`rust-version` in `Cargo.toml`]: https://doc.rust-lang.org/cargo/reference/manifest.html?highlight=pack#the-rust-version-field
This PR makes
enumerate_required_extensions
return the same type that is expected byvk::InstanceCreateInfoBuilder::enabled_extension_names
to slightly reduce the boilerplate needed to write small Vulkan apps.