[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 13/33] serial: start making SerialMM a sysbus device
From: |
Peter Maydell |
Subject: |
Re: [PATCH v3 13/33] serial: start making SerialMM a sysbus device |
Date: |
Wed, 20 Nov 2019 10:58:01 +0000 |
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