| Message ID | alpine.DEB.2.22.394.2403121725370.853156@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | docs/misra: add Rule 16.4 | expand |
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. > 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". Jan
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).
On 14.03.2024 00:04, Stefano Stabellini wrote: > On Wed, 13 Mar 2024, Jan Beulich wrote: >> On 13.03.2024 01:28, Stefano Stabellini wrote: >>> + 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). Hmm, my -0.5 was on the understanding that we would not encourage entirely mechanical adding of such comments. Yet providing a pre-worded comment here does exactly this, in my opinion. Imo here it should be made clear _where_ the comment needs to be put, but not how it is to be worded. As was (largely) settled on during the discussion, the comment doesn't need to go into a great level of detail. Jan
Hi Stefano, > On 14 Mar 2024, at 00:04, Stefano Stabellini <sstabellini@kernel.org> 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
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index 1e134ccebc..5e9367f11c 100644 --- 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) 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; + * - `Rule 16.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_02.c>`_ - Required - A switch label shall only be used when the most closely-enclosing
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- I tested the rendered output and it is correct both on the gitlab UI as well as with tools like rst2html. --- docs/misra/rules.rst | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)