On 7/6/22, Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> wrote: > On Wed, Jul 6, 2022 at 3:26 PM Martin Fernandez > <martin.fernandez@xxxxxxxxxxxxx> wrote: >> >> 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. >> > > Well, the purpose of the checkpatch documentation is to provide some > more background information on the warning (e.g., the historic > motivation, what to consider when judging its validity) and any hints > on possible resolutions. So, I would expect to see the documentation > cover explaining the most common (reusable) resolutions. A valid > argument why a check warning can be ignored falls into such a > resolution. In fact, the category "CHECK" in checkpatch.pl already > suggests that often the resolution may be to "inspect some code, but > not modify the code and then further 'ignore' the reported warning", > as some rules in checkpatch are checking something with just some > quite weak heuristics. > > So, for this patch here: How about avoiding the word "workaround" and > just state these to options as resolution, e.g., a text like this: > > Here are two possible options: > - Check the macro arguments of all uses of this macro to be free of > unintended side effects. Passing a constant value is usually fine, as > the compiler will use constant propagation and further optimizations > to produce acceptable code. > - If needed, define local variables in the macro to hold the macro's > argument. See the implementation of `min` in include/linux/minmax.h as > one example of this option. > > What do you think? Yep, that's very good. I'll go with it. > I really appreciate you providing some documentation for this rule. I > also appreciate the rules that checkpatch.pl checks being better > explained to all of us in the kernel community. That avoids that we > all, especially newcomers, individually wonder about what checkpatch > intends to warn us about. > > Lukas >
