[Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Markus Armbruster]

Commit fecd264 added a number of fall-throughs, but neglected to
properly document them as intentional.  Commit d922445 cleaned that up
for many, but not all cases.  Take care of the remaining ones.

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target-mips/translate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index e9d86b2..8abc12b 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -18729,6 +18729,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
     case OPC_SWL:
     case OPC_SWR:
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
+         /* fall through */
     case OPC_SB ... OPC_SH:
     case OPC_SW:
          gen_st(ctx, op, rt, rs, imm);
@@ -18817,6 +18818,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         case OPC_PS_FMT:
             check_cp1_enabled(ctx);
             check_insn_opc_removed(ctx, ISA_MIPS32R6);
+            /* fall through */
         case OPC_S_FMT:
         case OPC_D_FMT:
             check_cp1_enabled(ctx);
@@ -19000,6 +19002,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
     case OPC_LDL ... OPC_LDR:
     case OPC_LLD:
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
+        /* fall through */
     case OPC_LWU:
     case OPC_LD:
         check_insn(ctx, ISA_MIPS3);
@@ -19008,6 +19011,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         break;
     case OPC_SDL ... OPC_SDR:
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
+        /* fall through */
     case OPC_SD:
         check_insn(ctx, ISA_MIPS3);
         check_mips_64(ctx);
-- 
1.9.3

Re: [Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Peter Maydell]

On 20 January 2015 at 09:59, Markus Armbruster <armbru@redhat.com> wrote:
> Commit fecd264 added a number of fall-throughs, but neglected to
> properly document them as intentional.  Commit d922445 cleaned that up
> for many, but not all cases.  Take care of the remaining ones.
>
> Spotted by Coverity.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target-mips/translate.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index e9d86b2..8abc12b 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -18729,6 +18729,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
>      case OPC_SWL:
>      case OPC_SWR:
>          check_insn_opc_removed(ctx, ISA_MIPS32R6);
> +         /* fall through */

Indent here seems to be out by one? The others look OK.

-- PMM

Re: [Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 20 January 2015 at 09:59, Markus Armbruster <armbru@redhat.com> wrote:
>> Commit fecd264 added a number of fall-throughs, but neglected to
>> properly document them as intentional.  Commit d922445 cleaned that up
>> for many, but not all cases.  Take care of the remaining ones.
>>
>> Spotted by Coverity.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  target-mips/translate.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/target-mips/translate.c b/target-mips/translate.c
>> index e9d86b2..8abc12b 100644
>> --- a/target-mips/translate.c
>> +++ b/target-mips/translate.c
>> @@ -18729,6 +18729,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
>>      case OPC_SWL:
>>      case OPC_SWR:
>>          check_insn_opc_removed(ctx, ISA_MIPS32R6);
>> +         /* fall through */
>
> Indent here seems to be out by one? The others look OK.

Sorry about that.  Fix up on commit, or would you like a respin?

Re: [Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Maciej W. Rozycki]

On Tue, 20 Jan 2015, Markus Armbruster wrote:

> >> diff --git a/target-mips/translate.c b/target-mips/translate.c
> >> index e9d86b2..8abc12b 100644
> >> --- a/target-mips/translate.c
> >> +++ b/target-mips/translate.c
> >> @@ -18729,6 +18729,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
> >>      case OPC_SWL:
> >>      case OPC_SWR:
> >>          check_insn_opc_removed(ctx, ISA_MIPS32R6);
> >> +         /* fall through */
> >
> > Indent here seems to be out by one? The others look OK.
> 
> Sorry about that.  Fix up on commit, or would you like a respin?

 It looks to me like this whole file requires reindentation, perhaps a 
mechanical update like that would be better.  There are many lines with 9 
leading spaces (`wc -l' tells me 20), that I deliberately left as they are 
with my recent patches in this area so as not to obfuscate semantic 
changes.  There may be some more issues.

  Maciej

Re: [Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Peter Maydell]

On 20 January 2015 at 18:16, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Tue, 20 Jan 2015, Markus Armbruster wrote:
>
>> >> diff --git a/target-mips/translate.c b/target-mips/translate.c
>> >> index e9d86b2..8abc12b 100644
>> >> --- a/target-mips/translate.c
>> >> +++ b/target-mips/translate.c
>> >> @@ -18729,6 +18729,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
>> >>      case OPC_SWL:
>> >>      case OPC_SWR:
>> >>          check_insn_opc_removed(ctx, ISA_MIPS32R6);
>> >> +         /* fall through */
>> >
>> > Indent here seems to be out by one? The others look OK.
>>
>> Sorry about that.  Fix up on commit, or would you like a respin?
>
>  It looks to me like this whole file requires reindentation, perhaps a
> mechanical update like that would be better.  There are many lines with 9
> leading spaces (`wc -l' tells me 20), that I deliberately left as they are
> with my recent patches in this area so as not to obfuscate semantic
> changes.  There may be some more issues.

In this particular case, this part of the file is fine and the
problem is simply that this patch as it stands introduces a single
line (the one above) that's not indented correctly. The only fix
required is to delete one space in the line added by the patch
(thus avoiding introducing any new misindented lines).

In the general case, we tend to not do whole-file reindentation,
because it breaks 'git blame' and similar tools. It's not an
outright ban, though -- I guess it comes down to a maintainer
judgement call whether they think the benefit outweighs the cost
for a particular bit of code.

-- PMM

Re: [Qemu-devel] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Maciej W. Rozycki]

On Tue, 20 Jan 2015, Peter Maydell wrote:

> In this particular case, this part of the file is fine and the
> problem is simply that this patch as it stands introduces a single
> line (the one above) that's not indented correctly. The only fix
> required is to delete one space in the line added by the patch
> (thus avoiding introducing any new misindented lines).

 Right!  That's what I've been following too -- not to introduce 
misindentation despite any surrounding lines suffering from this problem.

> In the general case, we tend to not do whole-file reindentation,
> because it breaks 'git blame' and similar tools. It's not an
> outright ban, though -- I guess it comes down to a maintainer
> judgement call whether they think the benefit outweighs the cost
> for a particular bit of code.

 Fair enough.  In some cases misindentation causes confusion, but here it 
does seem to be the case.

  Maciej

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-mips: Clean up switch fall through after commit fecd264

[Michael Tokarev]

20.01.2015 12:59, Markus Armbruster wrote:
> Commit fecd264 added a number of fall-throughs, but neglected to
> properly document them as intentional.  Commit d922445 cleaned that up
> for many, but not all cases.  Take care of the remaining ones.

Applied to -trivial fixing one extra space on the way.

Thanks,

/mjt