On 7/6/22, Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> wrote: > 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". You are right. >> + 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. Good point. > 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? Yes, that's what I did on my patch that triggered this patch. But I don't think that's a workaround. You still have the issue there, just that the uses of the macros are "good". I think that falls better into the "I know what I'm doing, I'm ok with the warning" scenario, than a proper workaround. > 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 >> >
