-
Notifications
You must be signed in to change notification settings - Fork 214
Add validate_with to derive(ULE) and derive(VarULE) #2733
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
base: main
Are you sure you want to change the base?
Conversation
#[repr(packed)] | ||
#[derive(ule::ULE, Copy, Clone)] | ||
#[derive(ule::ULE, Copy, Clone, Debug)] | ||
#[ule(validate_with = "validate_foo_ule")] |
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.
nit: this should be zerovec::ule
. we do that consistently elsewhere.
It's also fine for it to be zerovec::validate_with
, or zerovec(validate_with =)
though that last pattern isn't used so far in this crate
@@ -275,3 +278,43 @@ pub fn extract_attributes_common( | |||
|
|||
Ok(attrs) | |||
} | |||
|
|||
pub(crate) fn find_validate_with_path( |
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.
issue: This should be a part of the attribute parsing above in extract_attributes_common
let validate_with = match utils::find_validate_with_path(&input.attrs, &attr_path) { | ||
Ok(Some(fn_path)) => { | ||
quote! { | ||
#fn_path(unsafe { Self::from_byte_slice_unchecked(bytes) })?; |
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.
thought: for ULE this should be free but for VarULE this will be a couple extra instructions. i suppose we don't care about that?
this way of doing it does make the derive code simpler.
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 are you comparing against?
Note that the case where validate_with
is not used, there is no code being added.
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.
oh, sorry, here we construct an &Self
(involving a minor amount of ptr metadata arithmetic) and immediately throw it away, we could theoretically merge it with the last_field_validator
but doing that consistently and well will be a mess so it doesn't really matter anyway
#1691
Note that Serde does this differently using
serde(try_from)
(see serde-rs/serde#939). However, since ULE types are dealt with by reference, and there isn't a safe way to transmute&A
to&Transparent<A>
, the try_from thing doesn't make sense for ULE. So, I made it avalidate_with
function instead.If we add a better way to go from
&A
to&Transparent<A>
, then we could use thetry_from
thing.