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

[Peter Maydell]

On Wed, 20 Nov 2019 at 10:40, Marc-André Lureau
<address@hidden> wrote:
> On Wed, Nov 20, 2019 at 2:36 PM Peter Maydell <address@hidden> wrote:
> > 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

In a typical device implementation, some functions take the
object as a "SerialMM" or other specific pointer. Some take
an Object*. Some take a DeviceState*. Some take void*.
Which of those is "the type of the object that is being implemented" ?

> > (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.

There is a clear convention, which I explained to you already
(variable is named with some abbreviation/initialism of
the type name, or sometimes just 's' for "state"). You are trying
to write code which ignores that convention and does something
else *which no other code is doing*. Please stop doing that -- in
particular, don't do it in one device in the middle of a refactoring
series.

If you want to argue that we should change our convention for
how we write QOM code, feel free -- but that should be a separate
discussion and probably ought to come with a plan for how
to do a general update of the codebase to avoid a weird
mix of styles.

thanks
-- PMM