Re: [PATCH] docs/misra: add Rule 16.4

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 14 Mar 2024, at 00:04, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 13 Mar 2024, Jan Beulich wrote:
>> On 13.03.2024 01:28, Stefano Stabellini wrote:
>>> --- a/docs/misra/rules.rst
>>> +++ b/docs/misra/rules.rst
>>> @@ -478,6 +478,30 @@ maintainers if you want to suggest a change.
>>>      - In addition to break, also other unconditional flow control 
>>> statements
>>>        such as continue, return, goto are allowed.
>>> 
>>> +   * - `Rule 16.4 
>>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_04.c>`_
>>> +     - Required
>>> +     - Every switch statement shall have a default label
>>> +     - Switch statements with enums as controlling expression don't need
>>> +       a default label as gcc -Wall enables -Wswitch which warns (and
>>> +       breaks the build)
>> 
>> I think this could do with mentioning -Werror.
> 
> No problem
> 
> 
>>> if one of the enum labels is missing from the
>>> +       switch.
>>> +
>>> +       Switch statements with integer types as controlling expression
>>> +       should have a default label:
>>> +
>>> +       - if the switch is expected to handle all possible cases
>>> +         explicitly, then a default label shall be added to handle
>>> +         unexpected error conditions, using BUG(), ASSERT(), WARN(),
>>> +         domain_crash(), or other appropriate methods;
>>> +
>>> +       - if the switch is expected to handle a subset of all
>>> +         possible cases, then a default label shall be added with an
>>> +         in-code comment as follows::
>>> +
>>> +             /* only handle a subset of the possible cases */
>>> +             default:
>>> +                 break;
>> 
>> Unless it being made crystal clear that mechanically reproducing this
>> comment isn't going to do, I'm going to have a hard time picking
>> between actively vetoing or just accepting if someone else acks this.
>> At the very least, though, the suggested (or, as requested, example)
>> comment should match ./CODING_STYLE. And it may need placing
>> differently if I understood correctly what Misra / Eclair demand
>> (between default: and break; rather than ahead of both).
>> 
>> The only place I'd accept a pre-cooked comment is to cover the
>> "notifier pattern".
> 
> Hi Jan, 
> 
> During the MISRA C call we discussed two distinct situations:
> 
> 1) when the default is not supposed to happen (it could be an BUG_ON)
> 2) when we only handle a subset of cases on purpose
> 
> For 2), it is useful to have a comment to clarify that we are dealing
> with 2) instead of 1). It is not a pre-cooked comment. The comment
> should be reviewed and checked that it is actually true that for this
> specific switch the default is expected and we should do nothing.
> 
> However, the comment is indeed pre-cooked in the sense that we don't
> need to have several different variations of them to explain why we only
> handle a subset of cases.
> 
> The majority of people on the call agreed with this (or so I understood).

In fact i do agree with Jan here somehow: we must not have a default comment
in the rules.rst.
It might be that a comment will look like that but I think it would be a 
mistake to
have a default one that people can just copy and paste without thinking.
I would suggest to put something like the following instead:
When a default is empty, a comment shall be added to state it with a reason
and when possible a more detailed explanation.

Regards
Bertrand