Re: [XEN PATCH v2 3/3] xen: address violations of MISRA C:2012 Rule 13.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.12.2023 04:22, Stefano Stabellini wrote:
> On Mon, 27 Nov 2023, Jan Beulich wrote:
>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>
>>> The assignment operation in:
>>>
>>> .irq = rc = uart->irq,
>>>
>>> is a persistent side effect in a struct initializer list.
>>>
>>> This patch avoids rc assignment and directly uses uart->irq
>>> in the following if statement.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@xxxxxxxxxxx>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
>>
>> Who's the author of this patch? (Either the order of the SoB is wrong, or
>> there's a From: tag missing.)
>>
>>> ---
>>> Changes in v2:
>>> - avoid assignment of rc;
>>> - drop changes in vcpu_yield(void).
>>> ---
>>>  xen/drivers/char/ns16550.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> This warrants a more specific subject prefix. Also there's only a single
>> violation being dealt with here.
>>
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -445,11 +445,13 @@ static void __init cf_check 
>>> ns16550_init_postirq(struct serial_port *port)
>>>              struct msi_info msi = {
>>>                  .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>                                   uart->ps_bdf[2]),
>>> -                .irq = rc = uart->irq,
>>> +                .irq = uart->irq,
>>>                  .entry_nr = 1
>>>              };
>>>  
>>> -            if ( rc > 0 )
>>> +            rc = 0;
>>> +
>>> +            if ( uart->irq > 0 )
>>>              {
>>>                  struct msi_desc *msi_desc = NULL;
>>
>> The fact that there's no functional change here isn't really obvious.
>> Imo you want to prove that to a reasonable degree in the description.
>  
> Agreed. Only reading this chunk, wouldn't it be better to do:
> 
>     };
> 
>     rc = uart->irq;
> 
>     if ( rc > 0 )
> 
> at least it would be obvious?

Indeed.

Jan