Re: [PATCH v3 13/33] serial: start making SerialMM a sysbus device

[Marc-André Lureau]

Hi

On Wed, Nov 20, 2019 at 2:36 PM Peter Maydell <address@hidden> wrote:
>
> On Wed, 20 Nov 2019 at 07:34, Marc-André Lureau
> <address@hidden> wrote:
>
> > > > +static void serial_mm_instance_init(Object *o)
> > > > +{
> > > > +    SerialMM *self = SERIAL_MM(o);
> > >
> > > 'self' is not idiomatic for the name of the variable containing
> > > the pointer to the object in QOM code ("git grep '\Wself\W' hw"
> > > shows no uses of it at all, which is quite unusual for us --
> > > usually the codebase has at least a few uses of any non-standard
> > > way of writing something ;-))
> > >
> > > Usually we use something approximating to the abbreviation
> > > of the type name, so here 'smm' would do.
> >
> > I would prefer something more straightforward than having to come up
> > with various name mangling.
> >
> > Imho, we loose some readability, consistency & semantic by not naming
> > the object argument of the method "self"
>
> There are two problems here:
>  (1) "self" gives no hint at all about whether it's the Object*,
> the DeviceState*, or the SerialMM*. All of those are the
> object the method is operating on, but the type differences matter.

"self" should have the type of the object that is being implemented.

serial_mm_instance_init ->  SerialMM *self

>
> (2) *Absolutely nobody anywhere else in any other device model
> is using the 'self' argument name*. It's not a convention if
> only this one file is using it, and adopting it here gives us
> absolutely no consistency -- exactly the opposite.

Since there is no clear convention, then adding "self" isn't breaking
any convention.

>
> Item (1) is part of why we do things the way we do; item (2)
> is why we should not make the 16550 UART some weird
> exception from the way we do things.

It's about trying to make things better, and not about staying in the
current undefined/free zone.

-- 
Marc-André Lureau