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),
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() }
}
};
}
--
2.46.2On 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>
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
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
>
>
© 2016 - 2026 Red Hat, Inc.