On 25/10/2023 15:44, Jan Beulich wrote:
On 25.10.2023 15:22, Nicola Vetrini wrote:
The clauses of a switch should be enclosed directly by a switch
statement to make the code more easily understandable and less
prone to errors.
Signed-off-by: Nicola Vetrini <nicola.vetr...@bugseng.com>
---
This patch is mainly indended to probe how the community, especially
the
maintainers, would receive such modifications to the code, and whether
there
would be consensus on the rule's adoption. Anyone is welcome to
give feedback on this, especially on the x86 side, where this pattern
is used more frequently.
The chosen instance (below) is one where it is relatively easy to argue
that putting the default label in an inner scope isn't much of a
difference as far as overall code size / redundancy is concerned. But
there are (perhaps many) other cases where the gains of using what
Misra dislikes are much higher.
This is another one of the various more recently discussed rules where
I think Misra is just going too far, dictating various aspects of style
for - in my personal view - no real gain. Furthermore, if you could
pick some more involved example (arch/x86/x86_emulate/x86_emulate.c may
yield a few "good" examples), I'd like to learn how you propose to
change such code, with two up-front constraints:
- no added redundancy,
- no new goto.
Either of them in replacement code would go against what the
description
above states as a goal.
Jan
Honestly, I think none of those is attainable in such cases as the ones
in x86_emulate.c
Take, for instance,
index 94caec1d142c..2a70c5f0a197 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1492,10 +1492,10 @@ x86_emulate(
if ( ops->rmw && dst.type == OP_MEM )
state->rmw = rmw_add;
else
- {
- case 0x02 ... 0x05: /* add */
emulate_2op_SrcV("add", src, dst, _regs.eflags);
- }
+ break;
+ case 0x02 ... 0x05: /* add */
+ emulate_2op_SrcV("add", src, dst, _regs.eflags);
break;
or
@@ -3504,13 +3504,16 @@ x86_emulate(
#if !defined(X86EMUL_NO_MMX) && !defined(X86EMUL_NO_SIMD)
case X86EMUL_OPC_66(0x0f, 0x2a): /* cvtpi2pd mm/m64,xmm */
- if ( ea.type == OP_REG )
- {
+ if ( ea.type == OP_REG ) {
+ host_and_vcpu_must_have(mmx);
+ op_bytes = (b & 4) && (vex.pfx & VEX_PREFIX_DOUBLE_MASK) ?
16 : 8;
+ goto simd_0f_fp;
+ }
+ break;
case X86EMUL_OPC(0x0f, 0x2a): /* cvtpi2ps mm/m64,xmm */
CASE_SIMD_PACKED_FP(, 0x0f, 0x2c): /* cvttp{s,d}2pi xmm/mem,mm
*/
CASE_SIMD_PACKED_FP(, 0x0f, 0x2d): /* cvtp{s,d}2pi xmm/mem,mm
*/
- host_and_vcpu_must_have(mmx);
- }
+ host_and_vcpu_must_have(mmx);
op_bytes = (b & 4) && (vex.pfx & VEX_PREFIX_DOUBLE_MASK) ? 16 :
8;
goto simd_0f_fp;
here, I think some amount of duplication is unavoidable.
Global deviation is quite a viable option, and in fact the one I would
favour the most.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
