Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

Hi Julien,

On 2023-11-07 13:44, Julien Grall wrote:
> Hi Nicola,
>
> On 07/11/2023 10:33, Nicola Vetrini wrote:
> > As explained in the deviation record, code constructs such as
> > "goto retry" and "goto again" are sometimes the best balance between
> > code complexity and the understandability of the control flow
> > by developers; as such, these construct are allowed to deviate
> > from Rule 15.2.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> >   automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++++++++++
> >   docs/misra/deviations.rst                        | 10 ++++++++++
> >   2 files changed, 20 insertions(+)
> >
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index fa56e5c00a27..8b1f622f8f82 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -270,6 +270,16 @@ statements are deliberate"
> >   -config=MC3R1.R14.3,statements={deliberate , 
> > "wrapped(any(),node(if_stmt))" }
> >   -doc_end
> >   +#
> > +# Series 15
> > +#
> > +
> > +-doc_begin="The additional complexity introduced in the code by using 
> > control flow structures other than backwards goto-s
> > +were deemed not to justify the possible prevention of developer 
> > confusion, given the very torough review process estabilished
> Typoes: s/torough/thorough/ s/estabilished/established/
Thanks

> > +in the community."
> > +-config=MC3R1.R15.2,reports+={deliberate, 
> > "any_area(any_loc(text(^.*goto (again|retry).*$)))"}
> > +-doc_end
> > +
> >   #
> >   # Series 20.
> >   #
> > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > index 8511a189253b..7d1e1f0d09b3 100644
> > --- a/docs/misra/deviations.rst
> > +++ b/docs/misra/deviations.rst
> > @@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
> >          statements are deliberate.
> >        - Project-wide deviation; tagged as `disapplied` for ECLAIR.
> >   +   * - R15.2
> > +     - The possible prevention of developer confusion as a result of 
> > using
> > +       control flow structures other than backwards goto-s in some 
> > instances was
> > +       deemed not strong enough to justify the additional complexity 
> > introduced
> > +       in the code. Such instances are the uses of the following 
> > labels:
> > +
> > +       - again
> > +       - retry
> Have you investigated the possibility to use SAF-X in the code? If so, 
> what's the problem to use them?
> Cheers,
This is another viable option: putting the SAF comment on top of the 
label should suffice,
as shown below:

/* SAF-2-safe */
repeat:
    ++fmt;          /* this also skips first '%' */
    switch (*fmt) {
    case '-': flags |= LEFT; goto repeat;
    case '+': flags |= PLUS; goto repeat;
    case ' ': flags |= SPACE; goto repeat;
    case '#': flags |= SPECIAL; goto repeat;
    case '0': flags |= ZEROPAD; goto repeat;
    }

I think it ultimately boils down to whether Xen wants to promote the use 
of certain labels
as the designated alternative when no other control flow mechanism is 
clearer from a
readability perspective (in which case there should be a consistent 
naming to capture and deviate
all of them, such as "retry") or do so on a case-by-case basis with a 
SAF, which is ok, but then
it needs someone to check each one and either fix them or mark them as 
ok.
Yet another route could be to mark with a SAF all those present right 
now to establish a baseline.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)