-
Notifications
You must be signed in to change notification settings - Fork 190
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
Assert that Vulkan array-getters return the same length #534
Conversation
ash/src/device.rs
Outdated
@@ -328,6 +328,7 @@ impl Device { | |||
&mut count, | |||
out.as_mut_ptr(), | |||
); | |||
assert_eq!(count, out.len() as u32); |
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.
could use as _
?
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.
What's the advantage of not being explicit about this (trivial to write) type?
That said, it's probably better to cast count
to usize
to prevent a possible truncation.
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 am always wondering wether to use as _
or be explicit...
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.
@filnet There are probably no real API guidelines but it's probably better to be explicit unless intentionally hiding long type casts.
I've also turned on the trivial-casts
and trivial-numereric-casts
default-allow
rustc
warning locally to notice that there are quite a few casts to the same type, mostly as _
which was copied verbatim without the programmer really knowing or caring about the source and target type.
In either case we should aim to flip that warning on and make the code ever so slightly more readable by removing stray as
casts.
That said, it's probably better to cast
count
tousize
to prevent a possible truncation.
On second thought I did this by intention to match the truncation from let mut count = out.len() as u32;
. However, that means - if someone manages to pass a slice longer than u32::max
at all - this assert won't fail even if it should. Erring on the side of correctness, this is now flipped.
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.
Built-in sanity-check asserts feels like it might be a bit opinionated compared to ash's status quo, but none of this is hotpath stuff anyway.
Originally introduced in [#489] this inserts the array-length equality check everywhere else: in the supposedly invalid and inexistant event where Vulkan suddenly returns less items (`count` has been modified) than were originally queried through respective `_len()` functions (or more likely: a slice of invalid length passed by the user) and some elements at the end of the slice are left uninitialized, panic. Wherever there is valid concern or possibility for this to happen - or to circumvent assertions and panics altogether - mutable references to mutable slices should be passed allowing the length to be promptly updated. [#489]: #489 (comment)
Originally introduced in #489 this inserts the array-length equality check everywhere else: in the supposedly invalid and inexistant event where Vulkan suddenly returns less items (
count
has been modified) than were originally queried through respective_len()
functions (or more likely: a slice of invalid length passed by the user) and some elements at the end of the slice are left uninitialized, panic.Wherever there is valid concern or possibility for this to happen - or to circumvent assertions and panics altogether - mutable references to mutable slices should be passed allowing the length to be promptly updated.