| Message ID | 20220802075433.1748035-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/char: imx-lpuart: Fix MISRA C 2012 Rule 20.7 violation | expand |
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". Jan
Hi Jan, 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) - is not justifiable (i.e does not fix a bug, does not result in more readable code etc) I understand that the rest of the community does not agree with adding parentheses around macro parameters used as function arguments and as the right-side operand of the assignment operator, but I consider them useful and I do not want to remove them myself. Maybe somebody else from the community could do that. Also, these exceptions would be good to be mentioned in the rules.rst. So, that other contributors do not try to fix relevant warnings.
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. > I understand that the rest of the community does not agree with adding > parentheses around macro parameters used as function arguments This may indeed be the case, while ... > and as > the right-side operand of the assignment operator, ... iirc for this one it was only Julien so far who disliked the addition of parentheses. > but I consider them > useful and I do not want to remove them myself. Maybe somebody else from > the community could do that. Fair enough; my remark was indeed _only_ a remark - the maintainers will have to judge in the end. Jan
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@kernel.org> 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.
On 03.08.2022 00:58, Stefano Stabellini wrote: > 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. I very specifically disagree with this statement: Attempts to add text there have been ignored altogether, i.e. have not even been seen worth a comment against the clarification and/or addition. I have therefore given up to propose changes to this document, and I'm also not going to suggest to anyone to make any attempt up and until I see movement on the adjustment proposals already pending (the two of them that I can easily locate for now over 2 years, and iirc there were more which predate our switching of email systems and which hence I wouldn't have in my outbox anymore). Jan
diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c index 2709136081..9c1f3b71a3 100644 --- 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)) static struct imx_lpuart { uint32_t baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
The macro parameter 'off' is used as an expression and it is good to be enclosed in parentheses to prevent against unintended expansion. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- xen/drivers/char/imx-lpuart.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)