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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?