| Message ID | 81ecc35d04456771b1e48cb25155b0151e2225b8.1712305581.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C Rule 16.2 | expand |
On 05.04.2024 11:14, Nicola Vetrini wrote:
> Remove unneded blank lines between switch clauses.
"Unneeded" based on what? We're carefully trying to improve readability of
large switch() statements by adding such blank lines (at least) between non-
fall-through case blocks, and you go and remove them?
Jan
On Mon, 8 Apr 2024, Jan Beulich wrote: > On 05.04.2024 11:14, Nicola Vetrini wrote: > > Remove unneded blank lines between switch clauses. > > "Unneeded" based on what? We're carefully trying to improve readability of > large switch() statements by adding such blank lines (at least) between non- > fall-through case blocks, and you go and remove them? To be fair it is almost impossible to figure out the correct coding style in Xen by looking at existing code and/or CODING_STYLE. Except for the blank lines, the change itself looks correct to me Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 2024-04-08 09:32, Jan Beulich wrote: > On 05.04.2024 11:14, Nicola Vetrini wrote: >> Remove unneded blank lines between switch clauses. > > "Unneeded" based on what? We're carefully trying to improve readability > of > large switch() statements by adding such blank lines (at least) between > non- > fall-through case blocks, and you go and remove them? > > Jan I wrote that based on this earlier suggestion [1]. If I misunderstood the suggestion, then I apologize and feel free to strip them if you want. [1] https://lore.kernel.org/xen-devel/e40579ba-acae-4c11-bea1-a5b83208db10@suse.com/
On 09/04/2024 8:45 pm, Nicola Vetrini wrote: > On 2024-04-08 09:32, Jan Beulich wrote: >> On 05.04.2024 11:14, Nicola Vetrini wrote: >>> Remove unneded blank lines between switch clauses. >> >> "Unneeded" based on what? We're carefully trying to improve >> readability of >> large switch() statements by adding such blank lines (at least) >> between non- >> fall-through case blocks, and you go and remove them? >> >> Jan > > I wrote that based on this earlier suggestion [1]. If I misunderstood > the suggestion, then I apologize and feel free to strip them if you want. > > [1] > https://lore.kernel.org/xen-devel/e40579ba-acae-4c11-bea1-a5b83208db10@suse.com/ I'm afraid I also can't figure out what that suggestion was supposed to be, but we definitely do want to keep blank lines. They're specifically for improved legibility. But fighting over spacing like this is a waste of everyone's time. I've taken patches 1 thru 7, accounting for the suggestions made so far, and adjusted to retain the blank lines. Please double check carefully. Patch 8 didn't apply because SAF-4-safe has been used for something else now. You'll need to rebase and resubmit patches 8 and 9. ~Andrew
On 2024-04-11 14:03, Andrew Cooper wrote: > On 09/04/2024 8:45 pm, Nicola Vetrini wrote: >> On 2024-04-08 09:32, Jan Beulich wrote: >>> On 05.04.2024 11:14, Nicola Vetrini wrote: >>>> Remove unneded blank lines between switch clauses. >>> >>> "Unneeded" based on what? We're carefully trying to improve >>> readability of >>> large switch() statements by adding such blank lines (at least) >>> between non- >>> fall-through case blocks, and you go and remove them? >>> >>> Jan >> >> I wrote that based on this earlier suggestion [1]. If I misunderstood >> the suggestion, then I apologize and feel free to strip them if you >> want. >> >> [1] >> https://lore.kernel.org/xen-devel/e40579ba-acae-4c11-bea1-a5b83208db10@suse.com/ > > I'm afraid I also can't figure out what that suggestion was supposed to > be, but we definitely do want to keep blank lines. They're > specifically > for improved legibility. > I interpreted that message as being a suggestion to eliminate blank lines, which was obviously incorrect. Anyways, thanks for the effort on adjusting and committing the earlier patches. > But fighting over spacing like this is a waste of everyone's time. > I've > taken patches 1 thru 7, accounting for the suggestions made so far, and > adjusted to retain the blank lines. > > Please double check carefully. > > Patch 8 didn't apply because SAF-4-safe has been used for something > else > now. You'll need to rebase and resubmit patches 8 and 9. > I'll certainly do so when I'm fully back to work next week. > ~Andrew
On 11.04.2024 14:03, Andrew Cooper wrote: > On 09/04/2024 8:45 pm, Nicola Vetrini wrote: >> On 2024-04-08 09:32, Jan Beulich wrote: >>> On 05.04.2024 11:14, Nicola Vetrini wrote: >>>> Remove unneded blank lines between switch clauses. >>> >>> "Unneeded" based on what? We're carefully trying to improve >>> readability of >>> large switch() statements by adding such blank lines (at least) >>> between non- >>> fall-through case blocks, and you go and remove them? >>> >>> Jan >> >> I wrote that based on this earlier suggestion [1]. If I misunderstood >> the suggestion, then I apologize and feel free to strip them if you want. >> >> [1] >> https://lore.kernel.org/xen-devel/e40579ba-acae-4c11-bea1-a5b83208db10@suse.com/ > > I'm afraid I also can't figure out what that suggestion was supposed to > be, The main point of missing clarity there is that we still need to settle on whether blank lines should also be between blocks where the earlier falls through to the latter. Iirc you'd like to have blank lines in such cases nevertheless, while I'd prefer to make the falling-through visually easy to recognize by not putting blanks lines between the "fallthrough;" / fall-through comment and the successive "case ...":. Jan > but we definitely do want to keep blank lines. They're specifically > for improved legibility.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index dcbcf4a1feb5..81efe5472518 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -976,7 +976,6 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) if ( val & ~APIC_TPRI_MASK ) return X86EMUL_EXCEPTION; break; - case APIC_SPIV: if ( val & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED | APIC_SPIV_FOCUS_DISABLED | @@ -984,38 +983,31 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) ? APIC_SPIV_DIRECTED_EOI : 0)) ) return X86EMUL_EXCEPTION; break; - case APIC_LVTT: if ( val & ~(LVT_MASK | APIC_TIMER_MODE_MASK) ) return X86EMUL_EXCEPTION; break; - case APIC_LVTTHMR: case APIC_LVTPC: case APIC_CMCI: if ( val & ~(LVT_MASK | APIC_DM_MASK) ) return X86EMUL_EXCEPTION; break; - case APIC_LVT0: case APIC_LVT1: if ( val & ~LINT_MASK ) return X86EMUL_EXCEPTION; break; - case APIC_LVTERR: if ( val & ~LVT_MASK ) return X86EMUL_EXCEPTION; break; - case APIC_TMICT: break; - case APIC_TDCR: if ( val & ~APIC_TDR_DIV_MASK ) return X86EMUL_EXCEPTION; break; - case APIC_ICR: if ( (uint32_t)val & ~(APIC_VECTOR_MASK | APIC_DM_MASK | APIC_DEST_MASK | APIC_INT_ASSERT | @@ -1023,21 +1015,19 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) return X86EMUL_EXCEPTION; vlapic_set_reg(vlapic, APIC_ICR2, val >> 32); break; - case APIC_SELF_IPI: if ( val & ~APIC_VECTOR_MASK ) return X86EMUL_EXCEPTION; offset = APIC_ICR; val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK); break; - case APIC_EOI: case APIC_ESR: - if ( val ) - { + if ( !val ) + break; + fallthrough; default: return X86EMUL_EXCEPTION; - } } vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val);
Remove unneded blank lines between switch clauses. Refactor the last clauses so that a violation of MISRA C Rule 16.2 is resolved (A switch label shall only be used when the most closely-enclosing compound statement is the body of a switch statement). The switch clause ending with the pseudo keyword "fallthrough" is an allowed exception to Rule 16.3. No functional change. Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/x86/hvm/vlapic.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)