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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 29 Jun 2023, at 20:20, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Thu, 29 Jun 2023, Luca Fancellu wrote:
>>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
>>> 
>>> In the files modified by this patch there are a few occurrences of nested 
>>> '//'
>>> character sequences inside C-style comment blocks, which violate Rule 3.1.
>>> The patch aims to resolve those by removing the nested comments.
>>> 
>>> In the file 'xen/common/xmalloc_tlsf.c' the comment has been replaces by
>> 
>> Typo: s/replaces/replaced/
>> 
>>> an ASSERT, to ensure that the invariant in the comment is enforced.
>>> 
>>> 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>
>> 
>> Same as patch 2, missing “---"
>> 
>>> Changes:
>>> - Resending the patch with the right maintainers in CC.
>>> Changes in V2:
>>> - Split the patch into a series and reworked the fix;
>>> - Apply the fix to the arm32 `flushtlb.h' file, for consistency.
>>> Changes in V3:
>>> - Replaced commmented-out 'if' with an ASSERT( *fl >= 0 ).
>>> ---
>>> xen/common/xmalloc_tlsf.c | 4 +---
>>> xen/include/xen/atomic.h  | 2 +-
>>> 2 files changed, 2 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>>> index 75bdf18c4e..95affcc571 100644
>>> --- a/xen/common/xmalloc_tlsf.c
>>> +++ b/xen/common/xmalloc_tlsf.c
>>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int 
>>> *fl, int *sl)
>>>        *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;
>>> -         */
>>> +        ASSERT( *fl >= 0 );
>> 
>> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it 
>> with spaces
>> before and after the condition (like our if conditions) so I think they can 
>> be dropped.
> 
> Yes, that's right. I am OK with this patch but I think we should wait
> for Jan's ack to be sure.
> 
> An alternative that I feel more comfortable in Acking myself because it
> doesn't change the semantics of this code would be to change the 3 lines
> of code above with this:
> 
> /*
> * ; FL will be always >0!
> * if ((*fl -= FLI_OFFSET) < 0)
> *     fl = *sl = 0;
> */

Hi Stefano,

I think we would have a violation for the Directive 4.4 (Sections of code 
should not be commented out)
in this case, however it’s not listed in rules.rst and I don’t remember where 
to look to know if it was
taken into consideration for the adoption in Xen in the “tailoring” phase.