| Message ID | 20241209123717.99077-27-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: bundle of prerequisites for HPET implementation | expand |
On Mon, Dec 09, 2024 at 01:37:17PM +0100, Paolo Bonzini wrote: > Date: Mon, 9 Dec 2024 13:37:17 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH 26/26] rust: callbacks: allow passing optional callbacks as > () > X-Mailer: git-send-email 2.47.1 > > In some cases, callbacks are optional. Using "Some(function)" and "None" > does not work well, because when someone writes "None" the compiler does > not know what to use for "F" in "Option<F>". I understand the direct use case is MemoryRegionOps, which has optional callbacks. However, I'm not quite sure how exactly it should be applied to C bindings and how it will play with Option<callback>. Could u pls provide a simple example? > Therefore, adopt () to mean a "null" callback. It is possible to enforce > that a callback is valid by adding a "let _: () = F::ASSERT_IS_SOME" before > the invocation of F::call. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/qemu-api/src/callbacks.rs | 97 ++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/rust/qemu-api/src/callbacks.rs b/rust/qemu-api/src/callbacks.rs > index 6401d807198..83c681d6478 100644 > --- a/rust/qemu-api/src/callbacks.rs > +++ b/rust/qemu-api/src/callbacks.rs > @@ -76,6 +76,31 @@ > /// call_it(&move |_| String::from(x), "hello workd"); typo: s/workd/word/ (in previous patch :-))
Il mar 17 dic 2024, 16:55 Zhao Liu <zhao1.liu@intel.com> ha scritto: > On Mon, Dec 09, 2024 at 01:37:17PM +0100, Paolo Bonzini wrote: > > Date: Mon, 9 Dec 2024 13:37:17 +0100 > > From: Paolo Bonzini <pbonzini@redhat.com> > > Subject: [PATCH 26/26] rust: callbacks: allow passing optional callbacks > as > > () > > X-Mailer: git-send-email 2.47.1 > > > > In some cases, callbacks are optional. Using "Some(function)" and "None" > > does not work well, because when someone writes "None" the compiler does > > not know what to use for "F" in "Option<F>". > > I understand the direct use case is MemoryRegionOps, which has optional > callbacks. However, I'm not quite sure how exactly it should be applied > to C bindings and how it will play with Option<callback>. > You wouldn't use Option<callback> at all, using () instead of None; the difference is that () does not have a parameter while None does (and the compiler cannot infer it). But I am okay with leaving this patch behind until there's a need. Paolo Could u pls provide a simple example? > > > Therefore, adopt () to mean a "null" callback. It is possible to enforce > > that a callback is valid by adding a "let _: () = F::ASSERT_IS_SOME" > before > > the invocation of F::call. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > rust/qemu-api/src/callbacks.rs | 97 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 97 insertions(+) > > > > diff --git a/rust/qemu-api/src/callbacks.rs b/rust/qemu-api/src/ > callbacks.rs > > index 6401d807198..83c681d6478 100644 > > --- a/rust/qemu-api/src/callbacks.rs > > +++ b/rust/qemu-api/src/callbacks.rs > > @@ -76,6 +76,31 @@ > > /// call_it(&move |_| String::from(x), "hello workd"); > > typo: s/workd/word/ (in previous patch :-)) > >
On Tue, Dec 17, 2024 at 05:40:14PM +0100, Paolo Bonzini wrote: > Date: Tue, 17 Dec 2024 17:40:14 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 26/26] rust: callbacks: allow passing optional > callbacks as () > > Il mar 17 dic 2024, 16:55 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > > On Mon, Dec 09, 2024 at 01:37:17PM +0100, Paolo Bonzini wrote: > > > Date: Mon, 9 Dec 2024 13:37:17 +0100 > > > From: Paolo Bonzini <pbonzini@redhat.com> > > > Subject: [PATCH 26/26] rust: callbacks: allow passing optional callbacks > > as > > > () > > > X-Mailer: git-send-email 2.47.1 > > > > > > In some cases, callbacks are optional. Using "Some(function)" and "None" > > > does not work well, because when someone writes "None" the compiler does > > > not know what to use for "F" in "Option<F>". > > > > I understand the direct use case is MemoryRegionOps, which has optional > > callbacks. However, I'm not quite sure how exactly it should be applied > > to C bindings and how it will play with Option<callback>. > > > > You wouldn't use Option<callback> at all, using () instead of None; the > difference is that () does not have a parameter while None does (and the > compiler cannot infer it). But I am okay with leaving this patch behind > until there's a need. Am I using the wrong terminology? Function pointers in a structure should be called a vtable, rather than callbacks (for example, methods in TypeInfo, read/write methods in MemoryRegionOps). Callbacks are typically function pointers used as function parameters (for example, timer/gpio). So, is the callback implementation here only used for the latter case? Thanks, Zhao
Il mer 18 dic 2024, 07:50 Zhao Liu <zhao1.liu@intel.com> ha scritto: > Am I using the wrong terminology? Function pointers in a structure should > be called a vtable, rather than callbacks (for example, methods in > TypeInfo, > read/write methods in MemoryRegionOps). Callbacks are typically function > pointers used as function parameters (for example, timer/gpio). So, is the > callback implementation here only used for the latter case? > The callback implementation is not used for QOM indeed. In that case, using FnCall would require something like const UNPARENT: impl FnCall((&Self,)); which does not exist as far as I know? MemoryRegionOps is a mix of callbacks and a vtable. From the Rust point of view, with the API that uses the builder pattern, MemoryRegionOps (and VMStateDescription too) would be closer to callbacks. Instead when you use traits and fill in the class object, that's clearly a vtable. But in this sense MemoryRegionOps do not need optional callbacks. You just don't call the ops.read() method if you don't need to set a read callback for example. So I am not sure if anything that is planned right now needs the optional callbacks. It's good to have the patch for the future but it's not necessary right now. Paolo
On Wed, Dec 18, 2024 at 08:32:59AM +0100, Paolo Bonzini wrote: > Date: Wed, 18 Dec 2024 08:32:59 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 26/26] rust: callbacks: allow passing optional > callbacks as () > > Il mer 18 dic 2024, 07:50 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > > Am I using the wrong terminology? Function pointers in a structure should > > be called a vtable, rather than callbacks (for example, methods in > > TypeInfo, > > read/write methods in MemoryRegionOps). Callbacks are typically function > > pointers used as function parameters (for example, timer/gpio). So, is the > > callback implementation here only used for the latter case? > > > > The callback implementation is not used for QOM indeed. In that case, using > FnCall would require something like > > const UNPARENT: impl FnCall((&Self,)); > > which does not exist as far as I know? Yes, it's incorrect. (I know you have some magic, so I have this question to see your idea.) > MemoryRegionOps is a mix of callbacks and a vtable. From the Rust point of > view, with the API that uses the builder pattern, MemoryRegionOps (and > VMStateDescription too) would be closer to callbacks. Instead when you use > traits and fill in the class object, that's clearly a vtable. > > But in this sense MemoryRegionOps do not need optional callbacks. You just > don't call the ops.read() method if you don't need to set a read callback > for example. So I am not sure if anything that is planned right now needs > the optional callbacks. It's good to have the patch for the future but it's > not necessary right now. Thanks for your explaination. I agree your plan. I also think it will be useful in the future. Regards, Zhao
diff --git a/rust/qemu-api/src/callbacks.rs b/rust/qemu-api/src/callbacks.rs index 6401d807198..83c681d6478 100644 --- a/rust/qemu-api/src/callbacks.rs +++ b/rust/qemu-api/src/callbacks.rs @@ -76,6 +76,31 @@ /// call_it(&move |_| String::from(x), "hello workd"); /// ``` /// +/// `()` can be used to indicate "no function": +/// +/// ``` +/// # use qemu_api::callbacks::FnCall; +/// fn optional<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> Option<String> { +/// if F::IS_SOME { +/// Some(F::call((s,))) +/// } else { +/// None +/// } +/// } +/// +/// assert!(optional(&(), "hello world").is_none()); +/// ``` +/// +/// Invoking `F::call` will then be a run-time error. +/// +/// ```should_panic +/// # use qemu_api::callbacks::FnCall; +/// # fn call_it<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> String { +/// # F::call((s,)) +/// # } +/// let s: String = call_it(&(), "hello world"); // panics +/// ``` +/// /// # Safety /// /// Because `Self` is a zero-sized type, all instances of the type are @@ -90,10 +115,70 @@ pub unsafe trait FnCall<Args, R = ()>: 'static + Sync + Sized { /// Rust 1.79.0+. const ASSERT_ZERO_SIZED: () = { assert!(mem::size_of::<Self>() == 0) }; + /// Referring to this constant asserts that the `Self` type is an actual + /// function type, which can be used to catch incorrect use of `()` + /// at compile time. + /// + /// # Examples + /// + /// ```compile_fail + /// # use qemu_api::callbacks::FnCall; + /// fn call_it<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> String { + /// let _: () = F::ASSERT_IS_SOME; + /// F::call((s,)) + /// } + /// + /// let s: String = call_it((), "hello world"); // does not compile + /// ``` + /// + /// Note that this use more simply `const { assert!(F::IS_SOME) }` in + /// Rust 1.79.0 or newer. + const ASSERT_IS_SOME: () = { assert!(Self::IS_SOME) }; + + /// `true` if `Self` is an actual function type and not `()`. + /// + /// # Examples + /// + /// You can use `IS_SOME` to catch this at compile time: + /// + /// ```compile_fail + /// # use qemu_api::callbacks::FnCall; + /// fn call_it<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> String { + /// const { assert!(F::IS_SOME) } + /// F::call((s,)) + /// } + /// + /// let s: String = call_it((), "hello world"); // does not compile + /// ``` + const IS_SOME: bool; + + /// `false` if `Self` is an actual function type, `true` if it is `()`. + fn is_none() -> bool { + !Self::IS_SOME + } + + /// `true` if `Self` is an actual function type, `false` if it is `()`. + fn is_some() -> bool { + Self::IS_SOME + } + /// Call the function with the arguments in args. fn call(a: Args) -> R; } +/// `()` acts as a "null" callback. Using `()` and `function` is nicer +/// than `None` and `Some(function)`, because the compiler is unable to +/// infer the type of just `None`. Therefore, the trait itself acts as the +/// option type, with functions [`FnCall::is_some`] and [`FnCall::is_none`]. +unsafe impl<Args, R> FnCall<Args, R> for () { + const IS_SOME: bool = false; + + /// Call the function with the arguments in args. + fn call(_a: Args) -> R { + panic!("callback not specified") + } +} + macro_rules! impl_call { ($($args:ident,)* ) => ( // SAFETY: because each function is treated as a separate type, @@ -103,6 +188,8 @@ unsafe impl<F, $($args,)* R> FnCall<($($args,)*), R> for F where F: 'static + Sync + Sized + Fn($($args, )*) -> R, { + const IS_SOME: bool = true; + #[inline(always)] fn call(a: ($($args,)*)) -> R { let _: () = Self::ASSERT_ZERO_SIZED; @@ -138,4 +225,14 @@ fn do_test_call<'a, F: FnCall<(&'a str,), String>>(_f: &F) -> String { fn test_call() { assert_eq!(do_test_call(&str::to_owned), "hello world") } + + // The `_f` parameter is unused but it helps the compiler infer `F`. + fn do_test_is_some<'a, F: FnCall<(&'a str,), String>>(_f: &F) { + assert_eq!(F::is_some(), true); + } + + #[test] + fn test_is_some() { + do_test_is_some(&str::to_owned); + } }
In some cases, callbacks are optional. Using "Some(function)" and "None" does not work well, because when someone writes "None" the compiler does not know what to use for "F" in "Option<F>". Therefore, adopt () to mean a "null" callback. It is possible to enforce that a callback is valid by adding a "let _: () = F::ASSERT_IS_SOME" before the invocation of F::call. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/qemu-api/src/callbacks.rs | 97 ++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)