| Message ID | 4981c3fb0992898a121881333485004f3609eaf7.1713866519.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN] automation/eclair: add deviations for MISRA C:2012 Rule 16.4 | expand |
On 23.04.2024 12:02, Federico Serafini wrote: > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -302,6 +302,19 @@ Deviations related to MISRA C:2012 Rules: > leave such files as is. > - Tagged as `deliberate` for ECLAIR. > > + * - R16.4 > + - Switch statements having a controlling expression of enum type > + deliberately do not have a default case: gcc -Wall enables -Wswitch > + which warns (and breaks the build as we use -Werror) if one of the enum > + labels is missing from the switch. > + - Tagged as `deliberate` for ECLAIR. > + > + * - R16.4 > + - A switch statement with a single switch clause and no default label may > + be used in place of an equivalent if statement if it is considered to > + improve readability." First a terminology related comment here: I'm afraid "switch clause" can be interpreted multiple ways, when I think we want to leave no room for interpretation here. It's not even clear to me whether switch ( x ) { case 1: case 2: case 3: case 4: ... break; } would be covered by the deviation, or whether the multiple case labels wouldn't already be too much. And then it is not clear to me why switch ( x ) { case 1: ... break; default: ... break; } shouldn't also be covered, as potentially a readability improvement / future change simplification over if ( x == 1 ) { ... } else { ... } Jan
On 23/04/24 12:26, Jan Beulich wrote: > On 23.04.2024 12:02, Federico Serafini wrote: >> --- a/docs/misra/deviations.rst >> +++ b/docs/misra/deviations.rst >> @@ -302,6 +302,19 @@ Deviations related to MISRA C:2012 Rules: >> leave such files as is. >> - Tagged as `deliberate` for ECLAIR. >> >> + * - R16.4 >> + - Switch statements having a controlling expression of enum type >> + deliberately do not have a default case: gcc -Wall enables -Wswitch >> + which warns (and breaks the build as we use -Werror) if one of the enum >> + labels is missing from the switch. >> + - Tagged as `deliberate` for ECLAIR. >> + >> + * - R16.4 >> + - A switch statement with a single switch clause and no default label may >> + be used in place of an equivalent if statement if it is considered to >> + improve readability." (I placed Rule 16.4 before Rule 16.3. I will propose a new version with the correct ordering.) > > First a terminology related comment here: I'm afraid "switch clause" can be > interpreted multiple ways, when I think we want to leave no room for > interpretation here. It's not even clear to me whether > > switch ( x ) > { > case 1: case 2: case 3: case 4: > ... > break; > } > > would be covered by the deviation, or whether the multiple case labels > wouldn't already be too much. The MISRA C document, within Rule 16.1 ("A switch statement shall be well-formed") defines the syntax rules that can be used to define a "well formed" switch statement. When I say "switch clause", I refer to the same entity the MISRA document refers to in the definition of such syntax rules. In the example above, we have a single switch clause with multiple labels and no default label: this is a violation of Rule 16.4 ("Every `switch' statement shall have a `default' label") which will be covered by the deviation. Do you think inserting the example in rules.rst or deviations.rst could be useful? > > And then it is not clear to me why > > switch ( x ) > { > case 1: > ... > break; > default: > ... > break; > } > > shouldn't also be covered, as potentially a readability improvement / > future change simplification over > > if ( x == 1 ) > { > ... > } > else > { > ... > } Here there are two switch clauses, each of them terminated by a break statement, and the default label is present: the switch is well formed, no violations of series 16 will be reported.
On 23.04.2024 17:52, Federico Serafini wrote: > On 23/04/24 12:26, Jan Beulich wrote: >> On 23.04.2024 12:02, Federico Serafini wrote: >>> --- a/docs/misra/deviations.rst >>> +++ b/docs/misra/deviations.rst >>> @@ -302,6 +302,19 @@ Deviations related to MISRA C:2012 Rules: >>> leave such files as is. >>> - Tagged as `deliberate` for ECLAIR. >>> >>> + * - R16.4 >>> + - Switch statements having a controlling expression of enum type >>> + deliberately do not have a default case: gcc -Wall enables -Wswitch >>> + which warns (and breaks the build as we use -Werror) if one of the enum >>> + labels is missing from the switch. >>> + - Tagged as `deliberate` for ECLAIR. >>> + >>> + * - R16.4 >>> + - A switch statement with a single switch clause and no default label may >>> + be used in place of an equivalent if statement if it is considered to >>> + improve readability." > > (I placed Rule 16.4 before Rule 16.3. > I will propose a new version with the correct ordering.) > >> >> First a terminology related comment here: I'm afraid "switch clause" can be >> interpreted multiple ways, when I think we want to leave no room for >> interpretation here. It's not even clear to me whether >> >> switch ( x ) >> { >> case 1: case 2: case 3: case 4: >> ... >> break; >> } >> >> would be covered by the deviation, or whether the multiple case labels >> wouldn't already be too much. > > The MISRA C document, within Rule 16.1 ("A switch statement shall be > well-formed") defines the syntax rules that can be used to define a > "well formed" switch statement. > When I say "switch clause", I refer to the same entity the MISRA > document refers to in the definition of such syntax rules. > In the example above, we have a single switch clause with multiple > labels and no default label: this is a violation of Rule 16.4 > ("Every `switch' statement shall have a `default' label") which will > be covered by the deviation. > Do you think inserting the example in rules.rst or deviations.rst could > be useful? No, I don't think there should be examples in those documents. But those documents should also not (blindly) rely on terminology in the Misra spec, as not everyone has access to that (licensed copies had to be obtained for quite a few of us). Jan
On 23/04/24 18:06, Jan Beulich wrote: > On 23.04.2024 17:52, Federico Serafini wrote: >> On 23/04/24 12:26, Jan Beulich wrote: >>> On 23.04.2024 12:02, Federico Serafini wrote: >>>> + >>>> + * - R16.4 >>>> + - A switch statement with a single switch clause and no default label may >>>> + be used in place of an equivalent if statement if it is considered to >>>> + improve readability." > > No, I don't think there should be examples in those documents. But those > documents should also not (blindly) rely on terminology in the Misra > spec, as not everyone has access to that (licensed copies had to be > obtained for quite a few of us). In deviations.rst there is an identical deviation for Rule 16.6 ("Every switch statement shall have at least two switch-clauses"). I think we should remain consistent.
On 24.04.2024 09:37, Federico Serafini wrote: > On 23/04/24 18:06, Jan Beulich wrote: >> On 23.04.2024 17:52, Federico Serafini wrote: >>> On 23/04/24 12:26, Jan Beulich wrote: >>>> On 23.04.2024 12:02, Federico Serafini wrote: >>>>> + >>>>> + * - R16.4 >>>>> + - A switch statement with a single switch clause and no default label may >>>>> + be used in place of an equivalent if statement if it is considered to >>>>> + improve readability." >> >> No, I don't think there should be examples in those documents. But those >> documents should also not (blindly) rely on terminology in the Misra >> spec, as not everyone has access to that (licensed copies had to be >> obtained for quite a few of us). > > In deviations.rst there is an identical deviation for Rule 16.6 > ("Every switch statement shall have at least two switch-clauses"). > I think we should remain consistent. Sure, I'm all for consistency. Yet given the term "switch clause" doesn't appear in the C standard (afaics), it wants defining somewhere. Jan
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index d21f112a9b..f09ad71acf 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -384,6 +384,14 @@ explicit comment indicating the fallthrough intention is present." -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through.? \\*/.*$,0..1))))"} -doc_end +-doc_begin="Switch statements having a controlling expression of enum type deliberately do not have a default case: gcc -Wall enables -Wswitch which warns (and breaks the build as we use -Werror) if one of the enum labels is missing from the switch." +-config=MC3R1.R16.4,reports+={deliberate,'any_area(kind(context)&&^.* has no `default.*$&&stmt(node(switch_stmt)&&child(cond,skip(__non_syntactic_paren_stmts,type(canonical(enum_underlying_type(any())))))))'} +-doc_end + +-doc_begin="A switch statement with a single switch clause and no default label may be used in place of an equivalent if statement if it is considered to improve readability." +-config=MC3R1.R16.4,switch_clauses+={deliberate,"switch(1)&&default(0)"} +-doc_end + -doc_begin="A switch statement with a single switch clause and no default label may be used in place of an equivalent if statement if it is considered to improve readability." -config=MC3R1.R16.6,switch_clauses+={deliberate, "default(0)"} -doc_end diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index ed0c1e8ed0..df87239b7d 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -302,6 +302,19 @@ Deviations related to MISRA C:2012 Rules: leave such files as is. - Tagged as `deliberate` for ECLAIR. + * - R16.4 + - Switch statements having a controlling expression of enum type + deliberately do not have a default case: gcc -Wall enables -Wswitch + which warns (and breaks the build as we use -Werror) if one of the enum + labels is missing from the switch. + - Tagged as `deliberate` for ECLAIR. + + * - R16.4 + - A switch statement with a single switch clause and no default label may + be used in place of an equivalent if statement if it is considered to + improve readability." + - Tagged as `deliberate` for ECLAIR. + * - R16.3 - Switch clauses ending with continue, goto, return statements are safe. - Tagged as `safe` for ECLAIR.
Update ECLAIR configuration to take into account the deviations agreed during MISRA meetings for Rule 16.4. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- automation/eclair_analysis/ECLAIR/deviations.ecl | 8 ++++++++ docs/misra/deviations.rst | 13 +++++++++++++ 2 files changed, 21 insertions(+)