| Message ID | 1421747963-2860-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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
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
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
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);
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(+)