Paolo Bonzini <[email protected]> writes:
> On 12/5/25 10:35, Markus Armbruster wrote:
>>> +/// A wrapper for a C `QObject`.
>>> +///
>>> +/// Because `QObject` is not thread-safe, the safety of these bindings
>>> +/// right now hinges on treating them as immutable. It is part of the
>>> +/// contract with the `QObject` constructors that the Rust struct is
>>> +/// only built after the contents are stable.
>>> +///
>>> +/// Only a bare bones API is public; production and consumption of
>>> `QObject`
>>> +/// generally goes through `serde`.
>>> +pub struct QObject(&'static UnsafeCell<bindings::QObject>);
>>
>> This defines the Rust QObject. All it contains is a reference (wrapped
>> in UnsafeCell) self.0 to the C QObject. Correct?
>
> Correct.
>
>>> +
>>> +// SAFETY: the QObject API are not thread-safe other than reference
>>> counting;
>>> +// but the Rust struct is only created once the contents are stable, and
>>> +// therefore it obeys the aliased XOR mutable invariant.
>>
>> In other words, we promise never to change a QObject while Rust code
>> holds a reference, except for the reference counts. Correct?
>>
>> The reference count is the mutable part of an otherwise immutable
>> object. Not mentioned here: it is atomic. Therefore, concurrent
>> updates cannot mess it up. Nothing depends on its value except
>> deallocation when the last reference drops. I figure that's why the
>> exception to "aliased XOR mutable" is fine. Correct?
>
> Yes, it's one of a few exceptions to "aliased XOR mutable" including:
>
> - Mutex (because only one guy can access it at all anyway)
>
> - RefCell (enforces aliased XOR mutable at run-time, enforces
> single-thread usage at compile-time)
>
> - atomics (a mini mutex)
>
> - Cell (Mutex:RefCell = atomics:Cell, in other words every access is
> independent but also single-thread usage is checked at compile time)
>
>>> +unsafe impl Send for QObject {}
>>> +unsafe impl Sync for QObject {}
>>> +
>>> +// Since a QObject can be a floating-point value, and potentially a NaN,
>>> +// do not implement Eq
>>> +impl PartialEq for QObject {
>>> + fn eq(&self, other: &Self) -> bool {
>>> + unsafe { bindings::qobject_is_equal(self.0.get(), other.0.get()) }
>>> + }
>>> +}
>>> +
>>> +impl QObject {
>>> + /// Construct a [`QObject`] from a C `QObjectBase` pointer.
>>
>> It's spelled QObjectBase_. More of the same below, not flagging again.
>>
>> Comment next to its definition:
>>
>> /* Not for use outside include/qobject/ */
>>
>> We're using it outside now. Do we really need to?
>
> It's because we're defining equivalents of inline functions in
> include/qobject.
Fair, but we should update comments when we make them wrong :)
> I can however replace uses of from_base with a macro similar to QOBJECT()
Might be cleaner.
>>> + /// Obtain a raw C pointer from a reference. `self` is consumed
>>> + /// and the C `QObject` pointer is leaked.
>>
>> What exactly do you mean by "leaked"?
>
> s/and the.*/without decreasing the reference count, thus transferring
> the reference to the `*mut bindings::QOjbect`/
Much clearer.
>>> + pub fn into_raw(self) -> *mut bindings::QObject {
>>> + let src = ManuallyDrop::new(self);
>>> + src.0.get()
>>> + }
>>> +
>>> + /// Construct a [`QObject`] from a C `QObject` pointer.
>>
>> Pasto? Isn't it QObjectBase_ here?
>
> Yes.
>
>>> +impl From<()> for QObject {
>>> + fn from(_null: ()) -> Self {
>>> + unsafe {
>>> QObject::cloned_from_base(addr_of!(bindings::qnull_.base)) }
>>
>> qnull_ is not meant for use outside qnull.[ch] and its unit test
>> check-qnull.c. Could we use qnull()?
>
> Same as above---it's inline. The above is a translation of
>
> static inline QNull *qnull(void)
> {
> return qobject_ref(&qnull_);
> }
Could we call C qnull() instead?
>>> +macro_rules! from_double {
>>> + ($t:ty) => {
>>> + impl From<$t> for QObject {
>>> + fn from(n: $t) -> Self {
>>> + let qobj = unsafe { &*bindings::qnum_from_double(n.into())
>>> };
>>> + unsafe { QObject::from_base(addr_of!(qobj.base)) }
>>> + }
>>> + }
>>> + };
>>> +}
>>> +
>>> +from_double!(f32);
>>
>> Uh, isn't the double in from_double misleading?
>
> It's a reference to the function that it calls (qnum_from_double). Can
> rename it to impl_from_returning_qnum_double.
>
>>> +from_double!(f64);
>>
>> Can you briefly explain why we need more than i64, u64, and double?
>
> Because Rust doesn't do automatic casts. So it's nicer (and also less
> error prone) if the subsequent patches do not have to always convert to
> u64 or i64.
Okay.
>> Skipping the remainder, it's too much macro magic for poor, ignorant me
>> :)
>
> It's not really hard. The thing to the left of => effectively defines a
> parser. Each thing of the shape $IDENT:RULE matches a piece of Rust
> grammar; expr is expression an tt is token tree (either a single token
> or a parenthesized group). To access $IDENT that appears within $(...)?
> on the left of => you must have a similar $(...)? on the right, and the
> whole $(...)? on the right will be skipped if the left-side wasn't there.
>
> The macro is used like this:
>
> match_qobject! { (self) =>
> () => Unexpected::Unit,
> bool(b) => Unexpected::Bool(b),
> i64(n) => Unexpected::Signed(n),
> u64(n) => Unexpected::Unsigned(n),
> f64(n) => Unexpected::Float(n),
> CStr(s) => s.to_str().map_or_else(
> |_| Unexpected::Other("string with invalid UTF-8"),
> Unexpected::Str),
> QList(_) => Unexpected::Seq,
> QDict(_) => Unexpected::Map,
> }
>
> And it produces a "switch" on QObject types, where each "case" extracts
> the datum, places it in the variable to the left of "=>" (such as "b"
> for bool), and returns the value on the right of "=>" (such as
> "Unexpected::Bool(b)"):
>
>
>>> + ) => {
>>> + loop {
>>> + let qobj_ = $qobj.0.get();
>>> + match unsafe { &* qobj_ }.base.type_ {
>>> + $($crate::bindings::QTYPE_QNULL => break $unit,)?
>>> + $($crate::bindings::QTYPE_QBOOL => break {
>>> + let qbool__: *mut $crate::bindings::QBool =
>>> qobj_.cast();
>>> + let $boolvar = unsafe { (&*qbool__).value };
>>> + $bool
>>> + },)?
>
> (The loop/break is just a syntactic convenience---the loop never rolls
> more than once).
>
> Paolo
Thanks for your help!
