Hi Aleksandar,

On 12/21/19 11:53 AM, Aleksandar Markovic wrote:


On Wednesday, December 18, 2019, Michael Rolnik <mrol...@gmail.com <mailto:mrol...@gmail.com>> wrote:

    This includes:
    - CPU data structures
    - object model classes and functions
    - migration functions
    - GDB hooks

    Co-developed-by: Michael Rolnik <mrol...@gmail.com
    <mailto:mrol...@gmail.com>>
    Co-developed-by: Sarah Harris <s.e.har...@kent.ac.uk
    <mailto:s.e.har...@kent.ac.uk>>
    Signed-off-by: Michael Rolnik <mrol...@gmail.com
    <mailto:mrol...@gmail.com>>
    Signed-off-by: Sarah Harris <s.e.har...@kent.ac.uk
    <mailto:s.e.har...@kent.ac.uk>>
    Signed-off-by: Michael Rolnik <mrol...@gmail.com
    <mailto:mrol...@gmail.com>>
    Acked-by: Igor Mammedov <imamm...@redhat.com
    <mailto:imamm...@redhat.com>>
    Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com
    <mailto:phi...@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.


Reply via email to