On 11.03.2024 10:02, Federico Serafini wrote:
> On 11/03/24 08:40, Jan Beulich wrote:
>> On 08.03.2024 12:51, Federico Serafini wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq)
>>>
>>> case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
>>> return arch_virq_is_global(virq);
>>> +
>>> + default:
>>> + ASSERT(virq < NR_VIRQS);
>>> + break;
>>> }
>>>
>>> - ASSERT(virq < NR_VIRQS);
>>> return true;
>>> }
>>
>> Just for my understanding: The ASSERT() is moved so the "default" would
>> consist of more than just "break". Why is it that then the "return" isn't
>> moved, too?
>
> No reason in particular.
> If preferred, I can move it too.
I for one would prefer that, yes. But what's needed up front is that we
decide what we want to do _consistently_ in all such cases.
>>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>> case ECS_VIRQ:
>>> printk(" v=%d", chn->u.virq);
>>> break;
>>> + default:
>>> + /* Nothing to do in other cases. */
>>> + break;
>>> }
>>
>> Yes this, just to mention it, while in line with what Misra demands is
>> pretty meaningless imo: The absence of "default" says exactly what the
>> comment now says. FTAOD - this is a comment towards the Misra guideline,
>> not so much towards the specific change here.
>
> Both you and Stefano reviewed the code and agreed on the fact that doing
> nothing for the default case is the right thing and now the code
> explicitly says that without letting any doubts.
> Furthermore, during the reviews it could happen that you notice a switch
> where something needs to be done for the default case.
That shouldn't happen during review. Anyone proposing a patch to add such
a comment wants to first have made sure the comment is actually applicable
there. Otherwise we're in "mechanically add comments" territory, which I
think we all agreed we want to avoid.
Jan
