| Message ID | 771a6f804f4e7dda3897359b57d1d14c2878ea16.1700844359.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address violations of MISRA C:2012 Rule 13.1 | expand |
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@bugseng.com> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> 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. Jan
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@bugseng.com> > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > 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?
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@bugseng.com> >>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> >> 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
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index ddf2a48be6..644a3192bb 100644 --- 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;