Re: [PATCH v39 01/22] target/avr: Add outward facing interfaces and core CPU logic - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Aleksandar Markovic <aleksandar.m.mail@gmail.com>,
	Michael Rolnik <mrolnik@gmail.com>
Cc: "thuth@redhat.com" <thuth@redhat.com>,
	Sarah Harris <S.E.Harris@kent.ac.uk>,
	"me@xcancerberox.com.ar" <me@xcancerberox.com.ar>,
	"richard.henderson@linaro.org" <richard.henderson@linaro.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"dovgaluk@ispras.ru" <dovgaluk@ispras.ru>,
	"imammedo@redhat.com" <imammedo@redhat.com>
Subject: Re: [PATCH v39 01/22] target/avr: Add outward facing interfaces and core CPU logic
Date: Sat, 21 Dec 2019 12:22:55 +0100	[thread overview]
Message-ID: <5e6bb53e-97ac-c630-c6c3-82cbd9657a79@redhat.com> (raw)
In-Reply-To: <CAL1e-=ikJOrT+ZCA+jjN0mj08uoeY7RpFqti=qE4RU+R+ikd2g@mail.gmail.com>

Hi Aleksandar,

On 12/21/19 11:53 AM, Aleksandar Markovic wrote:
> 
> 
> On Wednesday, December 18, 2019, Michael Rolnik <mrolnik@gmail.com 
> <mailto:mrolnik@gmail.com>> wrote:
> 
>     This includes:
>     - CPU data structures
>     - object model classes and functions
>     - migration functions
>     - GDB hooks
> 
>     Co-developed-by: Michael Rolnik <mrolnik@gmail.com
>     <mailto:mrolnik@gmail.com>>
>     Co-developed-by: Sarah Harris <S.E.Harris@kent.ac.uk
>     <mailto:S.E.Harris@kent.ac.uk>>
>     Signed-off-by: Michael Rolnik <mrolnik@gmail.com
>     <mailto:mrolnik@gmail.com>>
>     Signed-off-by: Sarah Harris <S.E.Harris@kent.ac.uk
>     <mailto:S.E.Harris@kent.ac.uk>>
>     Signed-off-by: Michael Rolnik <mrolnik@gmail.com
>     <mailto:mrolnik@gmail.com>>
>     Acked-by: Igor Mammedov <imammedo@redhat.com
>     <mailto:imammedo@redhat.com>>
>     Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>       target/avr/cpu-param.h |  37 +++
>       target/avr/cpu-qom.h   |  54 ++++
>       target/avr/cpu.h       | 258 ++++++++++++++++
>       target/avr/cpu.c       | 654 +++++++++++++++++++++++++++++++++++++++++
>       target/avr/gdbstub.c   |  84 ++++++
>       target/avr/machine.c   | 121 ++++++++
>       gdb-xml/avr-cpu.xml    |  49 +++
>       7 files changed, 1257 insertions(+)
>       create mode 100644 target/avr/cpu-param.h
>       create mode 100644 target/avr/cpu-qom.h
>       create mode 100644 target/avr/cpu.h
>       create mode 100644 target/avr/cpu.c
>       create mode 100644 target/avr/gdbstub.c
>       create mode 100644 target/avr/machine.c
>       create mode 100644 gdb-xml/avr-cpu.xml
> 
[...]
>     +typedef enum AVRFeature {
>     +    AVR_FEATURE_SRAM,
>     +
>     +    AVR_FEATURE_1_BYTE_PC,
>     +    AVR_FEATURE_2_BYTE_PC,
>     +    AVR_FEATURE_3_BYTE_PC,
>     +
>     +    AVR_FEATURE_1_BYTE_SP,
>     +    AVR_FEATURE_2_BYTE_SP,
>     +
>     +    AVR_FEATURE_BREAK,
>     +    AVR_FEATURE_DES,
>     +    AVR_FEATURE_RMW, /* Read Modify Write - XCH LAC LAS LAT */
>     +
>     +    AVR_FEATURE_EIJMP_EICALL,
>     +    AVR_FEATURE_IJMP_ICALL,
>     +    AVR_FEATURE_JMP_CALL,
>     +
>     +    AVR_FEATURE_ADIW_SBIW,
>     +
>     +    AVR_FEATURE_SPM,
>     +    AVR_FEATURE_SPMX,
>     +
>     +    AVR_FEATURE_ELPMX,
>     +    AVR_FEATURE_ELPM,
>     +    AVR_FEATURE_LPMX,
>     +    AVR_FEATURE_LPM,
>     +
>     +    AVR_FEATURE_MOVW,
>     +    AVR_FEATURE_MUL,
>     +    AVR_FEATURE_RAMPD,
>     +    AVR_FEATURE_RAMPX,
>     +    AVR_FEATURE_RAMPY,
>     +    AVR_FEATURE_RAMPZ,
>     +} AVRFeature;
>     +
> 
> 
> Very good! Now, each feature should have some really brief (less than 
> say 45 characters) comment in the same line as the definition, like this 
> one:
> 
>          AVR_FEATURE_MOVW,      /* supports MOVW instruction */

IMHO this looks overkill when the name is already obvious. In some cases 
we can explicit, such 'RMW'.

Maybe we can simply group the features by type with a single comment 
previous the group, such:

           /* Instructions */
           AVR_FEATURE_MOVW,
           AVR_FEATURE_MUL,
           AVR_FEATURE_RMW, /* Read Modify Write - XCH LAC LAS LAT */
           ...

> 
> You already did it for AVR_FEATURE_RMW
> 
> Of course, all such comments should be vertically aligned nicely.