-
Notifications
You must be signed in to change notification settings - Fork 184
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
Implement Bake
for VZV types of different formats
#5719
Conversation
@@ -291,6 +291,11 @@ pub mod vecs { | |||
pub use crate::varzerovec::{VarZeroSlice, VarZeroVec}; | |||
|
|||
pub use crate::varzerovec::{Index16, Index32, Index8, VarZeroVecFormat, VarZeroVecOwned}; | |||
|
|||
pub type VarZeroVec16<'a, T> = VarZeroVec<'a, T, Index16>; |
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: not sure I like these reexports.
} else { | ||
let bytes = databake::Bake::bake(&self.as_bytes(), env); | ||
// Safe because self.as_bytes is a safe input | ||
quote! { unsafe { zerovec::VarZeroVec::from_bytes_unchecked(#bytes) } } | ||
quote! { unsafe { zerovec::vecs::VarZeroVec16::from_bytes_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.
nit: we should just use VZV here, the generic gets defaulted
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: we could implement for all formats and then do a typeid type check to emit the compact thing for VZV16
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.
It doesn't, that's the problem. It gets inferred, so it's very unsafe to not specify it.
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.
@robertbastian hmm, I guess.
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.
sold on the safety angle
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.
LGTM. I've experienced hard-to-debug safety issues involving VZVs that change between index types. I like the re-exports because it keeps the AST the same size.
No description provided.