Re: [XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 12 Jul 2023, at 17:17, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
> 
> 
> 
> On 12/07/23 18:02, Luca Fancellu wrote:
>>> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
>>> 
>>> In the file 'xen/common/xmalloc_tlsf.c' is not clear how
>>> the commented-out code should interact with the previous statement.
>>> To resolve the MISRA violation generated by the nested comment
>>> a #if .. #endif block with an explanatory comment substitutes
>>> the earlier construct.
>>> 
>>> In the file 'xen/include/xen/atomic.h' the nested comment has been removed,
>>> since the code sample is already explained by the preceding comment.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>> ---
>>> Following the suggestion of this message
>>> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@xxxxxxxx/
>>> an explanatory comment has been added.
>>> ---
>>> xen/common/xmalloc_tlsf.c | 13 ++++++++++---
>>> xen/include/xen/atomic.h  |  2 +-
>>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>>> index c21bf71e88..56c3849414 100644
>>> --- a/xen/common/xmalloc_tlsf.c
>>> +++ b/xen/common/xmalloc_tlsf.c
>>> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, 
>>> int *fl, int *sl)
>>>         *r = *r + t;
>>>         *fl = flsl(*r) - 1;
>>>         *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>> -        *fl -= FLI_OFFSET;
>>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>>> -         *fl = *sl = 0;
>>> +        /*
>>> +         * It's unclear what was the purpose of the commented-out code 
>>> that now
>>> +         * is in the #else branch. The current form is motivated by the 
>>> correction
>>> +         * of a violation MISRA:C 2012 Rule 3.1
>>>          */
>>> +#if 1
>>> +        *fl -= FLI_OFFSET;
>>> +#else
>>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> In the message you linked above, you suggested to use /* FL will be always 
>> >0! */, why has it changed?
>> Was some comment I missed? The xen codestyle mandates the use of /* */, 
>> anyway I agree that here you
>> are just moving code...
>> So maybe the maintainer can tell what is the best thing to do here.
> 
> You didn't miss any further comment: my suggestion was related to the 
> explanatory comment, not the nested comment itself. If a better wording can 
> be found for the former, no problem. As for the codestyle point: it does not 
> change anything doing
> "// FL will be always >0!" -> "/* FL will be always >0!  */
> w.r.t. Rule 3.1 (both would have been nested comments).

Yes, I agree it does not change anything, now that I read better the message, 
it is from Jan suggesting this:

#if 1
    *fl -= FLI_OFFSET;
#else
    if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */
        *fl = *sl = 0;
#endif

So using /* FL will be always >0! */ instead of copying // FL will be always >0!

Anyway, I think it can be addressed on commit, whatever form the maintainer 
prefers.

> 
> Regards,
> 
> -- 
> Nicola Vetrini, BSc
> Software Engineer, BUGSENG srl (https://bugseng.com)