On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez <martin.fernandez@xxxxxxxxxxxxx> wrote: > > Add a description, an example and a possible workaround to the > MACRO_ARG_REUSE check. > > Signed-off-by: Martin Fernandez <martin.fernandez@xxxxxxxxxxxxx> > Acked-by: Dwaipayan Ray <dwaipayanray1@xxxxxxxxx> > --- > Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst > index b52452bc2963..86545c65cf7b 100644 > --- a/Documentation/dev-tools/checkpatch.rst > +++ b/Documentation/dev-tools/checkpatch.rst > @@ -759,6 +759,26 @@ Indentation and Line Breaks > Macros, Attributes and Symbols > ------------------------------ > > + **ARG_REUSE** The name of this checkpatch type is actually "MACRO_ARG_REUSE". > + Using the same argument multiple times in the macro definition > + would lead to unwanted side-effects. how about "... may lead to unwanted side effects"? Rationale: it does only lead to side effects if there are multiple computations involved. just on spelling: s/side-effects/side effects/ > + > + For example, given a `min` macro defined like:: > + > + #define min(x, y) ((x) < (y) ? (x) : (y)) > + > + If you call it with `min(foo(x), 0)`, it would expand to:: > + > + foo(x) < 0 ? foo(x) : 0 > + > + If `foo` has side-effects or it's an expensive calculation the > + results might not be what the user intended. > + s/side-effects/side effects/ > + For a workaround the idea is to define local variables to hold the > + macro's arguments. Checkout the actual implementation of `min` in > + include/linux/minmax.h for the full implementation of the > + workaround. > + I ran checkpatch on all commits from v5.17..v5.18 and looked for all check warnings with MACRO_ARG_REUSE. There were 35 warnings in 15 commits, touching 16 different files (4 in drivers/staging, 5 in drivers/net/wireless/, 5 in drivers/net/ethernet/, 1 in drivers/net/dsa/, 1 in drivers/net/can/). As far as I see it from those commits, the more common way to address this is to check that a macro is only used locally in some file and that all uses of that macro pass a constant value as macro argument. Maybe we add these two as equally good alternatives? Lukas > **ARRAY_SIZE** > The ARRAY_SIZE(foo) macro should be preferred over > sizeof(foo)/sizeof(foo[0]) for finding number of elements in an > -- > 2.30.2 >
