Message ID | 20200910194903.4104696-1-ehabkost@redhat.com |
---|---|
Headers | show |
Series | chardev: QOM cleanups | expand |
Hi On Fri, Sep 11, 2020 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com> > > wrote: > > > > > Some chardev QOM cleanup patches had to be dropped from my queue > > > due to build erros introduced by code movements across ifdef > > > boundaries at char-parallel.c. This series redo the changes from > > > those patches, but the macro renames are now a little different: > > > > > > In this version I have decided to rename the type checking macros > > > from *_CHARDEV to CHARDEV_* instead of renaming tye > > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the > > > identifiers actually match the QOM type name strings > > > ("chardev-*"). > > > > > > > Sounds reasonable to me, but it loses the matching with the > > structure/object type name though. > > > > - MuxChardev *d = MUX_CHARDEV(s); > > + MuxChardev *d = CHARDEV_MUX(s); > > > > I have a preference for the first. Unless we rename all the chardev types > > too... > > I tend to think the structs should be renamed too - I've always considerd > them to be backwards. > > > Imho, the QOM type name is mostly an internal detail, the C type name is > > dominant in the code. > > Err it is the reverse. The QOM type name is exposed public API via QOM > commands, while the C struct names are a internal private detail. > > Yes, but without the chardev- prefix (unless you try object-add which I don't think will work with chardev)
On Fri, Sep 11, 2020 at 09:10:18AM +0100, Daniel P. Berrangé wrote: > On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com> > > wrote: > > > > > Some chardev QOM cleanup patches had to be dropped from my queue > > > due to build erros introduced by code movements across ifdef > > > boundaries at char-parallel.c. This series redo the changes from > > > those patches, but the macro renames are now a little different: > > > > > > In this version I have decided to rename the type checking macros > > > from *_CHARDEV to CHARDEV_* instead of renaming tye > > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the > > > identifiers actually match the QOM type name strings > > > ("chardev-*"). > > > > > > > Sounds reasonable to me, but it loses the matching with the > > structure/object type name though. > > > > - MuxChardev *d = MUX_CHARDEV(s); > > + MuxChardev *d = CHARDEV_MUX(s); > > > > I have a preference for the first. Unless we rename all the chardev types > > too... > > I tend to think the structs should be renamed too - I've always considerd > them to be backwards. FWIW, "MuxChardev" sounds better to me. Not a big deal, though. (Also, I am not planning to touch any struct names for the sake of the new QOM declaration/definition macros. Renaming the type checking functions is enough churn.) > > > Imho, the QOM type name is mostly an internal detail, the C type name is > > dominant in the code. > > Err it is the reverse. The QOM type name is exposed public API via QOM > commands, while the C struct names are a internal private detail. I agree with Marc-André here. The C code is not just a detail. Code needs be easy to read, change and refactor. I'd really prefer to not have the external public API forcing a specific internal naming style. We have at least one case where it's probably going to be impossible to keep an exact match between the QOM type and type checker functions: "accel"/ACCEL. https://lore.kernel.org/qemu-devel/CAFEAcA9WEjne5TfwggVWPuBprkRs-a2-iNc43Xa_jBamaf9t8A@mail.gmail.com/