Provide CStr getters and setters for c_char pointers and arrays
#831
Conversation
MarijnS95
force-pushed
the
cstr-read-write-wrappers
branch
from
a5dc83c to
1006322
Compare
Pushed a second commit that returns a |
MarijnS95
force-pushed
the
cstr-read-write-wrappers
branch
3 times, most recently
from
4dd4d37 to
5df2380
Compare
|
IMO the |
It is a common operation to read and write NUL-terminated C string in the Vulkan API, yet the only helpers for that were thus far open-coded in the `Debug` printing implementations. Move them to separate functions that are exposed to the user, in hopes of helping them no longer misunderstand NUL-terminated strings (see e.g. #830). Important to note is that the array-copy for a static-sized `c_char` array has also been replaced with a `CStr` wrapper: this forces the user and our implementation to have a NUL-terminator at the end of the string, and the setter returns `Err()` when the given `CStr (with NUL-terminator) is too large for the static-sized array it has to be written to.
MarijnS95
force-pushed
the
cstr-read-write-wrappers
branch
from
5df2380 to
5e4ac22
Compare
|
Ah fair enough, I hadn't seen that yet but I agree; corrected that. Bit unfortunate that the standard library didn't follow this convention with |
| @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Added `VK_EXT_hdr_metadata` device extension (#804) | |||
| - Added `VK_NV_cuda_kernel_launch` device extension (#805) | |||
| - Added `descriptor_count()` setter on `ash::vk::WriteDescriptorSet` (#809) | |||
| - Added `*_as_c_str()` getters for `c_char` pointers and `c_char` arrays (#831) | |||
There was a problem hiding this comment.
Nit in hindsight: we also added setters for c_char pointers... And only replaced the c_char array setters with a CStr interface.
| #[inline] | ||
| #deprecated | ||
| pub unsafe fn #param_ident_as_c_str(&self) -> &std::ffi::CStr { | ||
| std::ffi::CStr::from_ptr(self.#param_ident) |
There was a problem hiding this comment.
@Ralith I don't remember if we discussed this but these pointers can trivially be NULL. Fortunately the function is unsafe and this call is not used in Debug, so I'm curious if it was deliberate.
However, we might want to switch this to return Option to represent that (and to safely use this API in Debug.
There was a problem hiding this comment.
Oops. I agree we should return Option here, and further, we should probably accept Option in the setters. Expecting users to check .foo.is_null() as appropriate is a bit optimistic and certain a missed opportunity for better safety.
I don't think we can safely use this API in Debug regardless because the pointer might be garbage.
There was a problem hiding this comment.
we should probably accept
Optionin the setters
Same for the pointer slice setters.
I don't think we can safely use this API in
Debugregardless because the pointer might be garbage.
Exactly, that's also what I wrote in #858 (comment): we can only print (subslices of) static arrays that are embedded in known-valid structs.
There was a problem hiding this comment.
Same for the pointer slice setters.
And arbitrary struct pointers (if Vulkan denotes them optional). Perhaps we should ask ourselves if the builder pattern isn't intended to (re)set fields to NULL?
There was a problem hiding this comment.
Good point. Let's not. The asymmetry is weird, but treating slices specially is weirder, and putting Option in every setter would be a significant downgrade in ergonomics.
There was a problem hiding this comment.
Seems reasonable, though of limited use outside implementing layers or ICDs.
There was a problem hiding this comment.
In hindsight it makes total sense, as the caller already allocates and passes those mutable arrays/sizes. That's not the case for the static-arrays in #858, so they're the only ones where a slice-getter makes sense.
I feel like I've brought this up before, and had the same realization only hours later. 🤯
There was a problem hiding this comment.
I'm confused -- are you saying it makes sense to not have getters for pointer-slice fields? Why would the storage not being inline make that any less useful?
There was a problem hiding this comment.
As you said, it's of limited use outside of layers and ICDs. Might be a useful addition if we wish to target those.
Just thinking out loud why I've never needed these slices helpers in client code 😉
There was a problem hiding this comment.
Ah, yeah. I know there's been occasional interest. We could have a feature gate for implementer-side APIs, even. Still, far from urgent.
It is a common operation to read and write NUL-terminated C string in the Vulkan API, yet the only helpers for that were thus far open-coded in the
Debugprinting implementations.Move them to separate functions that are exposed to the user, in hopes of helping them no longer misunderstand NUL-terminated strings (see e.g. #830).
Important to note is that the array-copy for a static-sized
c_chararray has also been replaced with aCStrwrapper: this forces the user and our implementation to have a NUL-terminated string, but may now also panic if the incomingCStrwith NUL-terminator is too large for the targetc_chararray.