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: Skip setters on returnedonly="true" structs #919

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Collaborator

Vulkan annotates structs that are purely returned by the driver and do not have to be constructed by users (besides initializing sType/ pNext). For these we can skip all builder functions (except push_next() and the CStr/slice getter helpers) and lighten our codebase somewhat.

Note that it is still possible to update the structure in a more low-level way by directly acessing the fields (or via FRU syntax). For Rust-based layer implementations this may be somewhat cumbersome, for which we could re-expose the setters behind a cfg(feature = "returnedonly-setters") of sorts?

@MarijnS95
Copy link
Collaborator Author

Note also that vk.xml retroactively adds this attribute to existing structures from time to time, because it was initially forgotten. That would immediately break semver compatibility for us so we have to be careful with this if we want to bump Vulkan spec versions without breaking ash releases.

@MarijnS95
Copy link
Collaborator Author

For completeness, note that upstream does not set returnedonly="true" for structs that require the user to allocate storage, like https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkSurfacePresentModeCompatibilityEXT.html / https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkLatencySurfaceCapabilitiesNV.html.

@Ralith
Copy link
Collaborator

Ralith commented May 15, 2024

For Rust-based layer implementations this may be somewhat cumbersome, for which we could re-expose the setters behind a cfg(feature = "returnedonly-setters") of sorts?

Sounds like a good idea to me. Perhaps output-setters?

For completeness, note that upstream does not set returnedonly="true" for structs that require the user to allocate storage

Hmm, seems like this should probably be an attribute on individual fields rather than entire structs.

@MarijnS95
Copy link
Collaborator Author

Hmm, seems like this should probably be an attribute on individual fields rather than entire structs.

Perhaps that's something @oddhack can help us out with, but I can also report it upstream. A lot of sType/pNext structs could have this attribute moved to all other fields.

Otherwise a caller-allocates like annotation/feature might be useful separately as well. Caller-allocated arrays aren't that common in structs (typically only reside in function parameters) but are a necessity for extendability for extending functions via pNext chains.

Vulkan annotates structs that are purely returned by the driver and
do not have to be constructed by users (besides initializing `sType`/
`pNext`).  For these we can skip all builder functions (except
`push_next()` and the `CStr`/slice getter helpers) and lighten our
codebase somewhat.

Note that it is still possible to update the structure in a more
low-level way by directly acessing the fields (or via FRU syntax).
For Rust-based layer implementations this may be somewhat cumbersome,
for which we could re-expose the setters behind a `cfg(feature =
"returnedonly-setters")` of sorts?
Comment on lines -33378 to -33383
#[inline]
pub fn data(mut self, data: &'a mut [u8]) -> Self {
self.data_size = data.len();
self.p_data = data.as_mut_ptr().cast();
self
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Edge-case: this is an out-array that the caller would have to allocate, by calling vkGetPipelineExecutableInternalRepresentationsKHR() twice with this set to NULL initially, and set to a valid mutable slice on the second call.

Copy link
Collaborator Author

@MarijnS95 MarijnS95 Dec 9, 2024

Choose a reason for hiding this comment

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

There appear to be 5 removed &'a mut Ts in this PR: those all need to remain for this change to be convenient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. Can we just check for mutable ref arguments?

Also, you previously noted:

note that upstream does not set returnedonly="true" for structs that require the user to allocate storage

Is that not true for these? Is the XML inconsistent here?

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.

3 participants