qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [PATCH] m68k comments break patch submission due to b


From: Laurent Vivier
Subject: Re: [Qemu-trivial] [PATCH] m68k comments break patch submission due to being incorrectly formatted
Date: Thu, 6 Jun 2019 19:00:20 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

CC Trivial

Le 06/06/2019 à 18:43, Lucien Murray-Pitts a écrit :
> Altering all comments in target/m68k to match Qemu coding styles so that 
> future
> patches wont fail due to style breaches.
> 
> Signed-off-by: Lucien Murray-Pitts <address@hidden>
> ---
>  target/m68k/cpu.c        |   4 +-
>  target/m68k/cpu.h        |  30 ++++--
>  target/m68k/fpu_helper.c |   6 +-
>  target/m68k/gdbstub.c    |   6 +-
>  target/m68k/helper.c     |  16 +--
>  target/m68k/m68k-semi.c  |  18 ++--
>  target/m68k/op_helper.c  |  54 ++++++----
>  target/m68k/softfloat.c  | 226 +++++++++++++++++++++++---------------
>  target/m68k/translate.c  | 227 ++++++++++++++++++++++++---------------
>  9 files changed, 365 insertions(+), 222 deletions(-)

Looks good, except:

> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index b16957934a..d12d906b11 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -204,8 +204,8 @@ static void any_cpu_initfn(Object *obj)
>      m68k_set_feature(env, M68K_FEATURE_CF_ISA_APLUSC);
>      m68k_set_feature(env, M68K_FEATURE_BRAL);
>      m68k_set_feature(env, M68K_FEATURE_CF_FPU);
> -    /* MAC and EMAC are mututally exclusive, so pick EMAC.
> -       It's mostly backwards compatible.  */
> +    /* MAC and EMAC are mututally exclusive, so pick EMAC. */
> +    /* It's mostly backwards compatible.  */

Use /*
     * MAC and EMAC are mututally exclusive, so pick EMAC.
     * It's mostly backwards compatible.
     */

>      m68k_set_feature(env, M68K_FEATURE_CF_EMAC);
>      m68k_set_feature(env, M68K_FEATURE_CF_EMAC_B);
>      m68k_set_feature(env, M68K_FEATURE_USP);
...
> diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
> index 1402145c8f..ed9245f366 100644
> --- a/target/m68k/m68k-semi.c
> +++ b/target/m68k/m68k-semi.c
> @@ -131,7 +131,8 @@ static void m68k_semi_return_u32(CPUM68KState *env, 
> uint32_t ret, uint32_t err)
>      target_ulong args = env->dregs[1];
>      if (put_user_u32(ret, args) ||
>          put_user_u32(err, args + 4)) {
> -        /* The m68k semihosting ABI does not provide any way to report this
> +        /*
> +         * The m68k semihosting ABI does not provide any way to report this
>           * error to the guest, so the best we can do is log it in qemu.
>           * It is always a guest error not to pass us a valid argument block.
>           */
> @@ -160,8 +161,8 @@ static void m68k_semi_cb(CPUState *cs, target_ulong ret, 
> target_ulong err)
>      CPUM68KState *env = &cpu->env;
>  
>      if (m68k_semi_is_fseek) {
> -        /* FIXME: We've already lost the high bits of the fseek
> -           return value.  */
> +        /* FIXME: We've already lost the high bits of the fseek */
> +        /* return value.  */

ditto like:

/*
 * FIXME: We've already lost the high bits of the fseek
 * return value.
 */

>          m68k_semi_return_u64(env, ret, err);
>          m68k_semi_is_fseek = 0;
>      } else {
> @@ -169,7 +170,8 @@ static void m68k_semi_cb(CPUState *cs, target_ulong ret, 
> target_ulong err)
>      }
>  }
>  
> -/* Read the input value from the argument block; fail the semihosting
> +/*
> + * Read the input value from the argument block; fail the semihosting
>   * call if the memory read fails.
>   */
>  #define GET_ARG(n) do {                                 \
> @@ -441,14 +443,14 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
>              }
>              ts->heap_limit = base + size;
>          }
> -        /* This call may happen before we have writable memory, so return
> -           values directly in registers.  */
> +        /* This call may happen before we have writable memory, so return */
> +        /* values directly in registers.  */
>          env->dregs[1] = ts->heap_limit;
>          env->aregs[7] = ts->stack_base;
>          }
>  #else
> -        /* FIXME: This is wrong for boards where RAM does not start at
> -           address zero.  */
> +        /* FIXME: This is wrong for boards where RAM does not start at */
> +        /* address zero.  */

ditto

>          env->dregs[1] = ram_size;
>          env->aregs[7] = ram_size;
>  #endif
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index bde2d551ff..0fa6f2c92e 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c

> @@ -838,14 +844,16 @@ static struct bf_data bf_prep(uint32_t addr, int32_t 
> ofs, uint32_t len)
>          addr -= 1;
>      }
>  
> -    /* Compute the number of bytes required (minus one) to
> -       satisfy the bitfield.  */
> +    /* Compute the number of bytes required (minus one) to */
> +    /* satisfy the bitfield.  */

ditto

>      blen = (bofs + len - 1) / 8;
>  
> -    /* Canonicalize the bit offset for data loaded into a 64-bit big-endian
> -       word.  For the cases where BLEN is not a power of 2, adjust ADDR so
> -       that we can use the next power of two sized load without crossing a
> -       page boundary, unless the field itself crosses the boundary.  */
> +    /*
> +     * Canonicalize the bit offset for data loaded into a 64-bit big-endian
> +     * word.  For the cases where BLEN is not a power of 2, adjust ADDR so
> +     * that we can use the next power of two sized load without crossing a
> +     * page boundary, unless the field itself crosses the boundary.
> +     */
>      switch (blen) {
>      case 0:
>          bofs += 56;
> @@ -937,8 +945,8 @@ uint64_t HELPER(bfextu_mem)(CPUM68KState *env, uint32_t 
> addr,
>      struct bf_data d = bf_prep(addr, ofs, len);
>      uint64_t data = bf_load(env, d.addr, d.blen, ra);
>  
> -    /* Put CC_N at the top of the high word; put the zero-extended value
> -       at the bottom of the low word.  */
> +    /* Put CC_N at the top of the high word; put the zero-extended value */
> +    /* at the bottom of the low word.  */

ditto

>      data <<= d.bofs;
>      data >>= 64 - d.len;
>      data |= data << (64 - d.len);
...
> diff --git a/target/m68k/softfloat.c b/target/m68k/softfloat.c
> index b45a5e8690..3cb1f494ba 100644
> --- a/target/m68k/softfloat.c
> +++ b/target/m68k/softfloat.c

In this file, it's better to remove the "/*------------------------" and
"*---------------------------------------*/" lines if you update them.

> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index f0534a4ba0..b796b6c22e 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -248,8 +248,8 @@ static void set_cc_op(DisasContext *s, CCOp op)
>      s->cc_op = op;
>      s->cc_op_synced = 0;
>  
> -    /* Discard CC computation that will no longer be used.
> -       Note that X and N are never dead.  */
> +    /* Discard CC computation that will no longer be used. */
> +    /* Note that X and N are never dead.  */

don't split in two comments.

>      dead = cc_op_live[old_op] & ~cc_op_live[op];
>      if (dead & CCF_C) {
>          tcg_gen_discard_i32(QREG_CC_C);
...

> @@ -2029,8 +2055,8 @@ DISAS_INSN(movem)
>              /* pre-decrement is not allowed */
>              goto do_addr_fault;
>          }
> -        /* We want a bare copy of the address reg, without any pre-decrement
> -           adjustment, as gen_lea would provide.  */
> +        /* We want a bare copy of the address reg, without any pre-decrement 
> */
> +        /* adjustment, as gen_lea would provide.  */

ditto

>          break;
>  
>      default:
...
> @@ -2938,8 +2972,8 @@ DISAS_INSN(jump)
>  {
>      TCGv tmp;
>  
> -    /* Load the target address first to ensure correct exception
> -       behavior.  */
> +    /* Load the target address first to ensure correct exception */
> +    /* behavior.  */

ditto

>      tmp = gen_lea(env, s, insn, OS_LONG);
>      if (IS_NULL_QREG(tmp)) {
>          gen_addr_fault(s);
> @@ -2976,8 +3010,8 @@ DISAS_INSN(addsubq)
>      dest = tcg_temp_new();
>      tcg_gen_mov_i32(dest, src);
>      if ((insn & 0x38) == 0x08) {
> -        /* Don't update condition codes if the destination is an
> -           address register.  */
> +        /* Don't update condition codes if the destination is an */
> +        /* address register.  */

ditto

>          if (insn & 0x0100) {
>              tcg_gen_sub_i32(dest, dest, val);
>          } else {

> @@ -4028,8 +4073,8 @@ DISAS_INSN(bfext_reg)
>              src = tmp;
>              pos = 32 - len;
>          } else {
> -            /* Immediate offset.  If the field doesn't wrap around the
> -               end of the word, rely on (s)extract completely.  */
> +            /* Immediate offset.  If the field doesn't wrap around the */
> +            /*   end of the word, rely on (s)extract completely.  */

ditto

>              if (pos < 0) {
>                  tcg_gen_rotli_i32(tmp, src, ofs);
>                  src = tmp;
...
> @@ -5635,8 +5686,8 @@ DISAS_INSN(mac)
>          TCGv rw;
>          rw = (insn & 0x40) ? AREG(insn, 9) : DREG(insn, 9);
>          tcg_gen_mov_i32(rw, loadval);
> -        /* FIXME: Should address writeback happen with the masked or
> -           unmasked value?  */
> +        /* FIXME: Should address writeback happen with the masked or */
> +        /*  unmasked value?  */

ditto

>          switch ((insn >> 3) & 7) {
>          case 3: /* Post-increment.  */
>              tcg_gen_addi_i32(AREG(insn, 0), addr, 4);
> @@ -5786,8 +5837,8 @@ register_opcode (disas_proc proc, uint16_t opcode, 
> uint16_t mask)
>                opcode, mask);
>        abort();
>    }
> -  /* This could probably be cleverer.  For now just optimize the case where
> -     the top bits are known.  */
> +  /* This could probably be cleverer.  For now just optimize the case where 
> */
> +  /*   the top bits are known.  */

ditto

>    /* Find the first zero bit in the mask.  */
>    i = 0x8000;
>    while ((i & mask) != 0)
> @@ -5805,17 +5856,20 @@ register_opcode (disas_proc proc, uint16_t opcode, 
> uint16_t mask)
>    }
>  }
>  
> -/* Register m68k opcode handlers.  Order is important.
> -   Later insn override earlier ones.  */
> +/*
> + * Register m68k opcode handlers.  Order is important.
> + * Later insn override earlier ones.
> + */
>  void register_m68k_insns (CPUM68KState *env)
>  {
> -    /* Build the opcode table only once to avoid
> -       multithreading issues. */
> +    /* Build the opcode table only once to avoid */
> +    /*   multithreading issues. */

ditto

>      if (opcode_table[0] != NULL) {
>          return;
>      }
>  
> -    /* use BASE() for instruction available
> +    /*
> +     * use BASE() for instruction available
>       * for CF_ISA_A and M68000.
>       */
>  #define BASE(name, opcode, mask) \

Thanks,
Laurent



reply via email to

[Prev in Thread] Current Thread [Next in Thread]