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

ash-window: Make enumerate_required_extensions return &[*const c_char] #590

Merged
merged 3 commits into from Mar 22, 2022

Conversation

swooster
Copy link
Contributor

This PR makes enumerate_required_extensions return the same type that is expected by vk::InstanceCreateInfoBuilder::enabled_extension_names to slightly reduce the boilerplate needed to write small Vulkan apps.

@swooster
Copy link
Contributor Author

Currently, the mismatch between what is returned by enumerate_required_extensions() and what is taken by vk::InstanceCreateInfoBuilder::enabled_extension_names() requires all Vulkan apps to write a bit of extra boilerplate to remap the list of extensions:

let extensions: Vec<_> = extensions
    .iter()
    .map(|ext| ext.as_ptr())
    .collect();

This PR will make that unnecessary.

@swooster swooster force-pushed the ash-window-cstr branch 2 times, most recently from e324255 to a66e39e Compare February 26, 2022 00:45
@swooster
Copy link
Contributor Author

Note: I don't think I've touched any of the areas with clippy warnings.

@swooster
Copy link
Contributor Author

swooster commented Feb 26, 2022

(rebased to incorporate #591 ... and fixed a relevant clippy warning now that the flood of others is gone)

Copy link
Collaborator

@Ralith Ralith left a 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!

ash-window/src/lib.rs Show resolved Hide resolved
@Ralith
Copy link
Collaborator

Ralith commented Feb 27, 2022

It should be noted that this will require ash-window's ash dependency to be collapsed down to the latest version once released.

@swooster
Copy link
Contributor Author

(rebasing onto latest commit)

@Ralith
Copy link
Collaborator

Ralith commented Feb 27, 2022

Just holding off on merging until @MarijnS95 has a look since it's breaking.

Copy link
Collaborator

@MarijnS95 MarijnS95 left a 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?

@swooster
Copy link
Contributor Author

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.

@MarijnS95
Copy link
Collaborator

@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 master and the open-PR-pipeline) that are non-breaking and would be interesting for a patch release. On the contrary, we have about 2-4 breaking changes queued up, so we might as well shoot for ash 0.37 breaking release straight away.

@MarijnS95 MarijnS95 changed the title Make ash-window's enumerate_required_extensions return &[*const c_char] ash-window: Make enumerate_required_extensions return &[*const c_char] Mar 22, 2022
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>.
@MarijnS95
Copy link
Collaborator

Rebased to regenerate against 1.3.208 :)

@MarijnS95
Copy link
Collaborator

@swooster Are you aware that your email address used in the commits isn't associated with your GitHub account?

@MarijnS95 MarijnS95 merged commit 1cd8106 into ash-rs:master Mar 22, 2022
@swooster
Copy link
Contributor Author

@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 (my username)+github@gmail.com and my commit is (my username)@gmail.com then that's ok. If they're substantially different, then that'd be a bad sign.

I've gone ahead and added (my username)@gmail.com to my github account.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Mar 23, 2022

@swooster it seems to be your initials + surname, separated with dots, at gmail dot com. See the Co-authored-by: field that GitHub generated for the merge commit 😉

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.

MarijnS95 added a commit to Traverse-Research/ash that referenced this pull request Mar 27, 2022
[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
Ralith pushed a commit that referenced this pull request Mar 27, 2022
)

[#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants