On Thu, 16 Feb 2023, Philippe Mathieu-Daudé wrote:
On 16/2/23 12:20, Thomas Huth wrote:
On 13/02/2023 19.43, Philippe Mathieu-Daudé wrote:
Use the PCI_SERIAL() QOM type-checking macro to avoid DO_UPCAST().
Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
---
hw/char/serial-pci.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 801b769aba..9689645cac 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -36,7 +36,10 @@
#include "qom/object.h"
struct PCISerialState {
+ /*< private >*/
PCIDevice dev;
+ /*< public >*/
+
I'm not sure about this part of the patch. It does not seem to be related
to the other changes at all, and are you sure about which parts are really
"public" and which parts are "private"? If so, I'd like to see a
description about this in the commit message, preferably in a separate
patch. Also, why an empty line after the "public" comment?
This is how QOM style separates the object 'private' part -- the
inherited parent, used by the QOM-cast macros -- and the fields
specific to this object.
The private field *must* be the first one in the structure for the
cast macros to work.
Maybe this isn't a convention and we could make one, to unify the
API style. I'm open to better suggestion :)
I suppose I got custom to see it to distinct the QOM hierarchy and
now it helps me to detect what is QOM and what isn't.
Anyway I'll remove from this patch.
I also dislike these comments and empty lines in these struct definitions.
I think it should be enough to document this QOM convention in the docs
saying that each QOM object state has to have it's parent's state as first
member and you're not supposed to access it directly (except maybe from
very closely related sub class) but do a QOM cast instead. If this is
clearly stated in the docs then there's no need to add comments about this
in every object. You could tell QOM objects from other structs by the
first member also being a QOM object and usually called parent or similar
but sometimes just dev. If you really want to get fancy maybe you could
hide it in a macro, something like:
OBJECT_STATE(PCISerialState, PCIDevice)
...
END_OBJECT_STATE
but I'm not sure I like that because it has to hide the braces in the
macro so it's not clear it's just a struct. So just describing it in the
docs if it's not already is probably enough.
Regards,
BALATON Zoltan
