> > > +// Some C users of this device embed its state struct into their own
> > > +// structs, so the size of the Rust version must not be any larger
> > > +// than the size of the C one. If this assert triggers you need to
> > > +// expand the padding_for_rust[] array in the C PL011State struct.
> > > +static_assert!(mem::size_of::<PL011State>() <= 
> > > mem::size_of::<bindings::PL011State>());
> > > +
> >
> > maybe use qemu_api::bindings::PL011State directly? Because bindings
> > contains native C structures/functions and their use should not be
> > encouraged, I think it's better to 'hide' bindings (not list it at the
> > beginning of the file).
> 
> Yeah, I wasn't sure what our preferred style approach is here
> regarding what we "use" and what we just directly reference
> (and the same in the other direction for mem::size_of vs
> size_of). Is there a "normal" pattern to follow here ?

There seems no clear doc on when to list use statements, but it's common
to list as clearly as possible to make it easier to sort out dependencies.

About bindings, I think it's better to clearly point out the specific
members in bindings, so ‘use qemu_api::bindings’ looks vague. Alternatively,
the qemu_api::bindings::PL011State could also be listed at the beginning of
the file, similar to a previous clean up: 06a1cfb5550a ("rust/pl011: Avoid
bindings::*") and another patch [1].

[1]: 
https://lore.kernel.org/qemu-devel/20250318130219.1799170-16-zhao1....@intel.com/

> Speaking of size_of, I noticed that Rust provides both
> core::mem::size_of and std::mem::size_of, and in rust/ at
> the moment we have uses of both. What's the difference?

They're the same (a simple proof of this is that the "source" option of
the std::mem page [2] points to the core::mem repo). `core` is
self-contained without OS dependency, and `std` is the superset of `core`
with extra OS dependency. And there's a previous cleanup to consolidate
`std::ptr` (commit c48700e86d, "rust: prefer importing std::ptr over
core::ptr"). So I think we can prefer std::mem as well.

[2]: https://doc.rust-lang.org/std/mem/index.html

Regards,
Zhao


Reply via email to