| Message ID | 20230213184338.46712-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Use QOM macros and remove DO_UPCAST() uses | expand |
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é <philmd@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? > SerialState state; > uint8_t prog_if; > }; > @@ -46,7 +49,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCISerialState, PCI_SERIAL) > > static void serial_pci_realize(PCIDevice *dev, Error **errp) > { > - PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev); > + PCISerialState *pci = PCI_SERIAL(dev); > SerialState *s = &pci->state; > > if (!qdev_realize(DEVICE(s), NULL, errp)) { > @@ -63,7 +66,7 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp) > > static void serial_pci_exit(PCIDevice *dev) > { > - PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev); > + PCISerialState *pci = PCI_SERIAL(dev); > SerialState *s = &pci->state; > > qdev_unrealize(DEVICE(s)); Ack for the DO_UPCAST removal. Thomas
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é <philmd@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. > Ack for the DO_UPCAST removal. Thanks!
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é <philmd@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
On 16/2/23 15:17, BALATON Zoltan wrote: > 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é <philmd@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. Well this and the new line somehow helps to not reorder this field elsewhere in the structure. > 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. Maybe "first_field_is_always_the_qom_parent" so nobody will be tempted to access it directly? :) > 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
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 >*/ + SerialState state; uint8_t prog_if; }; @@ -46,7 +49,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCISerialState, PCI_SERIAL) static void serial_pci_realize(PCIDevice *dev, Error **errp) { - PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev); + PCISerialState *pci = PCI_SERIAL(dev); SerialState *s = &pci->state; if (!qdev_realize(DEVICE(s), NULL, errp)) { @@ -63,7 +66,7 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp) static void serial_pci_exit(PCIDevice *dev) { - PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev); + PCISerialState *pci = PCI_SERIAL(dev); SerialState *s = &pci->state; qdev_unrealize(DEVICE(s));
Use the PCI_SERIAL() QOM type-checking macro to avoid DO_UPCAST(). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/char/serial-pci.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)