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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@bugseng.com>
>> ---
>>   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)