| Message ID | 20241021163538.136941-10-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: miscellaneous cleanups + QOM integration tests | expand |
Am 21.10.2024 um 18:35 hat Paolo Bonzini geschrieben: > Use the "struct update" syntax to initialize most of the fields to zero, > and simplify the handmade type-checking of $name. > > Reviewed-by: Junjie Mao <junjie.mao@hotmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/qemu-api/src/device_class.rs | 29 ++++++----------------------- > 1 file changed, 6 insertions(+), 23 deletions(-) > > diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs > index 2219b9f73d0..5aba426d243 100644 > --- a/rust/qemu-api/src/device_class.rs > +++ b/rust/qemu-api/src/device_class.rs > @@ -29,44 +29,27 @@ macro_rules! device_class_init { > macro_rules! define_property { > ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => { > $crate::bindings::Property { > - name: { > - #[used] > - static _TEMP: &::core::ffi::CStr = $name; > - _TEMP.as_ptr() > - }, > + // use associated function syntax for type checking > + name: ::core::ffi::CStr::as_ptr($name), I like this part. > info: $prop, > offset: ::core::mem::offset_of!($state, $field) > .try_into() > .expect("Could not fit offset value to type"), > - bitnr: 0, > - bitmask: 0, > set_default: true, > defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() }, > - arrayoffset: 0, > - arrayinfo: ::core::ptr::null(), > - arrayfieldsize: 0, > - link_type: ::core::ptr::null(), > + ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() } But is it really worth introducing unsafe code just for a more compact notation? If the compiler doesn't actually understand the pattern, it might even be less efficient than what we had (i.e. if it really creates the zeroed object and copies stuff over). Kevin
Il mar 22 ott 2024, 21:46 Kevin Wolf <kwolf@redhat.com> ha scritto: > > info: $prop, > > offset: ::core::mem::offset_of!($state, $field) > > .try_into() > > .expect("Could not fit offset value to type"), > > - bitnr: 0, > > - bitmask: 0, > > set_default: true, > > defval: $crate::bindings::Property__bindgen_ty_1 { u: > $defval.into() }, > > - arrayoffset: 0, > > - arrayinfo: ::core::ptr::null(), > > - arrayfieldsize: 0, > > - link_type: ::core::ptr::null(), > > + ..unsafe { > ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() > } > > But is it really worth introducing unsafe code just for a more compact > notation? If the compiler doesn't actually understand the pattern, it > might even be less efficient than what we had (i.e. if it really creates > the zeroed object and copies stuff over). > It goes away later in the series (patch 11 replaces it with a more manageable "Zeroable::ZERO"), but I wanted to split the patches in "parts that change existing code" and "parts that introduce new concepts". I agree that it's not optimal either way, I went like this because at this point this "unsafe { zeroed() }" idiom is present several times in the code and one more doesn't really change much. Paolo > Kevin > >
Hello Paolo, On Mon, 21 Oct 2024 19:35, Paolo Bonzini <pbonzini@redhat.com> wrote: >Use the "struct update" syntax to initialize most of the fields to zero, >and simplify the handmade type-checking of $name. Note: It wasn't meant for type checking but for making sure the linker doesn't strip the symbol (hence the #[used] attribute). These were left over when I was debugging linker issues and slapped #[used] everywhere but they are not needed in this case indeed. > >Reviewed-by: Junjie Mao <junjie.mao@hotmail.com> >Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >--- > rust/qemu-api/src/device_class.rs | 29 ++++++----------------------- > 1 file changed, 6 insertions(+), 23 deletions(-) > >diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs >index 2219b9f73d0..5aba426d243 100644 >--- a/rust/qemu-api/src/device_class.rs >+++ b/rust/qemu-api/src/device_class.rs >@@ -29,44 +29,27 @@ macro_rules! device_class_init { > macro_rules! define_property { > ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => { > $crate::bindings::Property { >- name: { >- #[used] >- static _TEMP: &::core::ffi::CStr = $name; >- _TEMP.as_ptr() >- }, >+ // use associated function syntax for type checking >+ name: ::core::ffi::CStr::as_ptr($name), > info: $prop, > offset: ::core::mem::offset_of!($state, $field) > .try_into() > .expect("Could not fit offset value to type"), >- bitnr: 0, >- bitmask: 0, > set_default: true, > defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() }, >- arrayoffset: 0, >- arrayinfo: ::core::ptr::null(), >- arrayfieldsize: 0, >- link_type: ::core::ptr::null(), >+ ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() } Call it personal taste but I don't like emulating C's zero initializer syntax in Rust :) Is it that much trouble to explicitly write down every field in a macro, anyway? No strong preference here though. > } > }; > ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => { > $crate::bindings::Property { >- name: { >- #[used] >- static _TEMP: &::core::ffi::CStr = $name; >- _TEMP.as_ptr() >- }, >+ // use associated function syntax for type checking >+ name: ::core::ffi::CStr::as_ptr($name), > info: $prop, > offset: ::core::mem::offset_of!($state, $field) > .try_into() > .expect("Could not fit offset value to type"), >- bitnr: 0, >- bitmask: 0, > set_default: false, >- defval: $crate::bindings::Property__bindgen_ty_1 { i: 0 }, >- arrayoffset: 0, >- arrayinfo: ::core::ptr::null(), >- arrayfieldsize: 0, >- link_type: ::core::ptr::null(), >+ ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() } > } > }; > } >-- >2.46.2 >
On 10/23/24 12:38, Manos Pitsidianakis wrote: > Hello Paolo, > > On Mon, 21 Oct 2024 19:35, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Use the "struct update" syntax to initialize most of the fields to zero, >> and simplify the handmade type-checking of $name. > > Note: It wasn't meant for type checking but for making sure the linker > doesn't strip the symbol (hence the #[used] attribute). These were left > over when I was debugging linker issues and slapped #[used] everywhere > but they are not needed in this case indeed. Well, it does do type checking as well, :) otherwise you end up duck-typing on whether $name as as_ptr(). I guess you are okay with the change below and the comment: >> - name: { >> - #[used] >> - static _TEMP: &::core::ffi::CStr = $name; >> - _TEMP.as_ptr() >> - }, >> + // use associated function syntax for type checking >> + name: ::core::ffi::CStr::as_ptr($name), ? >> info: $prop, >> offset: ::core::mem::offset_of!($state, $field) >> .try_into() >> .expect("Could not fit offset value to type"), >> - bitnr: 0, >> - bitmask: 0, >> set_default: true, >> defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() }, >> - arrayoffset: 0, >> - arrayinfo: ::core::ptr::null(), >> - arrayfieldsize: 0, >> - link_type: ::core::ptr::null(), >> + ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() } > > Call it personal taste but I don't like emulating C's zero initializer > syntax in Rust :) Is it that much trouble to explicitly write down every > field in a macro, anyway? No strong preference here though. Rust does make generous use of "..Default::default()", so I think it's more idiomatic to use the struct update syntax. We just cannot use it here because it's not const-ified. I'm okay with switching from Zeroable::ZERO to something like ConstDefault::CONST_DEFAULT; it's basically just a rename. On the other hand you do rely on "zero-ness" in PL011State::init()... so I thought I was actually following your style. :) Paolo
On Mon, Oct 21, 2024 at 06:35:34PM +0200, Paolo Bonzini wrote: > Date: Mon, 21 Oct 2024 18:35:34 +0200 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH v2 09/13] rust: clean up define_property macro > X-Mailer: git-send-email 2.46.2 > > Use the "struct update" syntax to initialize most of the fields to zero, > and simplify the handmade type-checking of $name. > > Reviewed-by: Junjie Mao <junjie.mao@hotmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/qemu-api/src/device_class.rs | 29 ++++++----------------------- > 1 file changed, 6 insertions(+), 23 deletions(-) Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs index 2219b9f73d0..5aba426d243 100644 --- a/rust/qemu-api/src/device_class.rs +++ b/rust/qemu-api/src/device_class.rs @@ -29,44 +29,27 @@ macro_rules! device_class_init { macro_rules! define_property { ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => { $crate::bindings::Property { - name: { - #[used] - static _TEMP: &::core::ffi::CStr = $name; - _TEMP.as_ptr() - }, + // use associated function syntax for type checking + name: ::core::ffi::CStr::as_ptr($name), info: $prop, offset: ::core::mem::offset_of!($state, $field) .try_into() .expect("Could not fit offset value to type"), - bitnr: 0, - bitmask: 0, set_default: true, defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() }, - arrayoffset: 0, - arrayinfo: ::core::ptr::null(), - arrayfieldsize: 0, - link_type: ::core::ptr::null(), + ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() } } }; ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => { $crate::bindings::Property { - name: { - #[used] - static _TEMP: &::core::ffi::CStr = $name; - _TEMP.as_ptr() - }, + // use associated function syntax for type checking + name: ::core::ffi::CStr::as_ptr($name), info: $prop, offset: ::core::mem::offset_of!($state, $field) .try_into() .expect("Could not fit offset value to type"), - bitnr: 0, - bitmask: 0, set_default: false, - defval: $crate::bindings::Property__bindgen_ty_1 { i: 0 }, - arrayoffset: 0, - arrayinfo: ::core::ptr::null(), - arrayfieldsize: 0, - link_type: ::core::ptr::null(), + ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() } } }; }