Re: [PATCH] xen/char: imx-lpuart: Fix MISRA C 2012 Rule 20.7 violation

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 2 Aug 2022, Jan Beulich wrote:
> On 02.08.2022 14:40, Xenia Ragiadakou wrote:
> > On 8/2/22 14:58, Jan Beulich wrote:
> >> On 02.08.2022 09:54, Xenia Ragiadakou wrote:
> >>> --- a/xen/drivers/char/imx-lpuart.c
> >>> +++ b/xen/drivers/char/imx-lpuart.c
> >>> @@ -26,8 +26,8 @@
> >>>   #include <asm/imx-lpuart.h>
> >>>   #include <asm/io.h>
> >>>   
> >>> -#define imx_lpuart_read(uart, off)       readl((uart)->regs + off)
> >>> -#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + 
> >>> off)
> >>> +#define imx_lpuart_read(uart, off)       readl((uart)->regs + (off))
> >>> +#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + 
> >>> (off))
> >>
> >> As elsewhere before I think at the same time you want to drop the
> >> parentheses from the single use of "val".
> >>
> > 
> > In general I do not want to include irrelevant changes in my patches.
> > Also, here, I do not want to drop the parentheses from val because the 
> > removal of the parentheses
> > - consists a violation of the rule 20.7
> > - would allow the following to compile
> > #define VAL x, y, z);(
> > imx_lpuart_write(uart, off, VAL)
> 
> Parenthesization won't help against all forms of odd use of parentheses
> in macro expansions anyway. Maybe MISRA should (or even does) have a
> rule disallowing unbalanced parentheses (an square brackets) in macro
> expansions ...
> 
> > - is not justifiable (i.e does not fix a bug, does not result in more 
> > readable code etc)
> 
> As said before, I very much view too many parentheses as affecting
> readability.

This patch is correct and it fixes the issue which is meant to fix.

Dropping the parentheses from the single use of "val" is not something
currently covered by our coding style document or by MISRA, so it is
normal that we are going to get a variety of opinions and preferences on
it.

I think it is better to avoid asking for changes not currently in
CODING_STYLE and docs/misra. It is less work for both reviewers and
contributors to add the rule to the coding style first, then ask for
changes.

So my preference is to keep this patch as is (regardless of what we are
going to do with "val"):

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


But I am happy to add the "avoid too many parenthesis" point to the list
of coding style things to discuss. FYI I paused the MISRA C discussion
meetings so that we can make some progress on fixing violations, but I
plan to resume those meetings in a month or two.