* [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE @ 2022-07-04 22:57 Martin Fernandez 2022-07-06 7:30 ` Bagas Sanjaya ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Martin Fernandez @ 2022-07-04 22:57 UTC (permalink / raw) To: linux-doc; +Cc: dwaipayanray1, lukas.bulwahn, joe, Martin Fernandez Add a description, an example and a possible workaround to the MACRO_ARG_REUSE check. Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> --- 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** + Using the same argument multiple times in the macro definition + would lead to unwanted 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. + + 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. + **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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-04 22:57 [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE Martin Fernandez @ 2022-07-06 7:30 ` Bagas Sanjaya 2022-07-06 13:19 ` Martin Fernandez 2022-07-06 11:29 ` Lukas Bulwahn 2022-07-06 15:09 ` Akira Yokosawa 2 siblings, 1 reply; 11+ messages in thread From: Bagas Sanjaya @ 2022-07-06 7:30 UTC (permalink / raw) To: Martin Fernandez; +Cc: linux-doc, dwaipayanray1, lukas.bulwahn, joe On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote: > + **ARG_REUSE** > + Using the same argument multiple times in the macro definition > + would lead to unwanted 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 > + Nit: literal blocks are indented three spaces relative to surrounding paragraph. > + If `foo` has side-effects or it's an expensive calculation the > + results might not be what the user intended. > + > + 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. > + For inline code, the correct syntax is ``some text``. However, by convention here, the backquotes aren't used where these would be appropriate, like variable and function names. For the last paragraph, better say "The workaround is to define local variables to hold macro arguments. See the min macro in include/linux/minmax.h for example.". -- An old man doll... just what I always wanted! - Clara ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 7:30 ` Bagas Sanjaya @ 2022-07-06 13:19 ` Martin Fernandez 2022-07-06 14:03 ` Akira Yokosawa 0 siblings, 1 reply; 11+ messages in thread From: Martin Fernandez @ 2022-07-06 13:19 UTC (permalink / raw) To: Bagas Sanjaya; +Cc: linux-doc, dwaipayanray1, lukas.bulwahn, joe On 7/6/22, Bagas Sanjaya <bagasdotme@gmail.com> wrote: > On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote: >> + **ARG_REUSE** >> + Using the same argument multiple times in the macro definition >> + would lead to unwanted 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 >> + > > Nit: literal blocks are indented three spaces relative to surrounding > paragraph. I just been told that I should be using 2 (I was using 1) and the rest of the file have 2 spaces... >> + If `foo` has side-effects or it's an expensive calculation the >> + results might not be what the user intended. >> + >> + 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. >> + > > For inline code, the correct syntax is ``some text``. You are right, I just misleadingly reused the syntax for some other example in the file. > However, by > convention here, the backquotes aren't used where these would be > appropriate, like variable and function names. So you are saying that for single variables and functions you don't use double backquotes? > For the last paragraph, better say "The workaround is to define local > variables to hold macro arguments. See the min macro in > include/linux/minmax.h for example.". I like it. Thanks. > -- > An old man doll... just what I always wanted! - Clara > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 13:19 ` Martin Fernandez @ 2022-07-06 14:03 ` Akira Yokosawa 2022-07-06 14:07 ` Martin Fernandez 0 siblings, 1 reply; 11+ messages in thread From: Akira Yokosawa @ 2022-07-06 14:03 UTC (permalink / raw) To: Martin Fernandez Cc: bagasdotme, dwaipayanray1, joe, linux-doc, lukas.bulwahn, Akira Yokosawa Hi, Let me chime in. On Wed, 6 Jul 2022 10:19:46 -0300, Martin Fernandez wrote: > On 7/6/22, Bagas Sanjaya <bagasdotme@gmail.com> wrote: >> On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote: >>> + **ARG_REUSE** >>> + Using the same argument multiple times in the macro definition >>> + would lead to unwanted 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 >>> + >> >> Nit: literal blocks are indented three spaces relative to surrounding >> paragraph. > > I just been told that I should be using 2 (I was using 1) and the rest > of the file have 2 spaces... I think what Bagas said above is convention of Python documentation [1]. As far I see, there is no such convention in kernel documentation. Indents of 2 spaces are fine as far as they are consistent in related .rst files, I suppose. [1]: https://devguide.python.org/documenting/#use-of-whitespace > >>> + If `foo` has side-effects or it's an expensive calculation the >>> + results might not be what the user intended. >>> + >>> + 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. >>> + >> >> For inline code, the correct syntax is ``some text``. > > You are right, I just misleadingly reused the syntax for some other > example in the file. > >> However, by >> convention here, the backquotes aren't used where these would be >> appropriate, like variable and function names. > > So you are saying that for single variables and functions you don't > use double backquotes? If you want crossref from the functions to their kernel-doc definitions, you can just say function() --- no double backquotes. If you say ``function()``, crossref won't work. See [2] for such crossrefs. [2]: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#cross-referencing-from-restructuredtext For simple variables, the style is up to you. Too much double backquotes might make the text hard to read as plain text, though. Thanks, Akira > >> For the last paragraph, better say "The workaround is to define local >> variables to hold macro arguments. See the min macro in >> include/linux/minmax.h for example.". > > I like it. Thanks. > >> -- >> An old man doll... just what I always wanted! - Clara >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 14:03 ` Akira Yokosawa @ 2022-07-06 14:07 ` Martin Fernandez 0 siblings, 0 replies; 11+ messages in thread From: Martin Fernandez @ 2022-07-06 14:07 UTC (permalink / raw) To: Akira Yokosawa; +Cc: bagasdotme, dwaipayanray1, joe, linux-doc, lukas.bulwahn On 7/6/22, Akira Yokosawa <akiyks@gmail.com> wrote: > Hi, > Let me chime in. > > On Wed, 6 Jul 2022 10:19:46 -0300, Martin Fernandez wrote: >> On 7/6/22, Bagas Sanjaya <bagasdotme@gmail.com> wrote: >>> On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote: >>>> + **ARG_REUSE** >>>> + Using the same argument multiple times in the macro definition >>>> + would lead to unwanted 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 >>>> + >>> >>> Nit: literal blocks are indented three spaces relative to surrounding >>> paragraph. >> >> I just been told that I should be using 2 (I was using 1) and the rest >> of the file have 2 spaces... > > I think what Bagas said above is convention of Python documentation [1]. > As far I see, there is no such convention in kernel documentation. > Indents of 2 spaces are fine as far as they are consistent in > related .rst files, I suppose. > > [1]: https://devguide.python.org/documenting/#use-of-whitespace > >> >>>> + If `foo` has side-effects or it's an expensive calculation the >>>> + results might not be what the user intended. >>>> + >>>> + 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. >>>> + >>> >>> For inline code, the correct syntax is ``some text``. >> >> You are right, I just misleadingly reused the syntax for some other >> example in the file. >> >>> However, by >>> convention here, the backquotes aren't used where these would be >>> appropriate, like variable and function names. >> >> So you are saying that for single variables and functions you don't >> use double backquotes? > > If you want crossref from the functions to their kernel-doc definitions, > you can just say function() --- no double backquotes. > If you say ``function()``, crossref won't work. See [2] for such > crossrefs. > > [2]: > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#cross-referencing-from-restructuredtext > > For simple variables, the style is up to you. Too much double > backquotes might make the text hard to read as plain text, though. Great! Thanks for clearing both doubts! > Thanks, Akira > >> >>> For the last paragraph, better say "The workaround is to define local >>> variables to hold macro arguments. See the min macro in >>> include/linux/minmax.h for example.". >> >> I like it. Thanks. >> >>> -- >>> An old man doll... just what I always wanted! - Clara >>> >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-04 22:57 [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE Martin Fernandez 2022-07-06 7:30 ` Bagas Sanjaya @ 2022-07-06 11:29 ` Lukas Bulwahn 2022-07-06 13:26 ` Martin Fernandez 2022-07-06 15:09 ` Akira Yokosawa 2 siblings, 1 reply; 11+ messages in thread From: Lukas Bulwahn @ 2022-07-06 11:29 UTC (permalink / raw) To: Martin Fernandez; +Cc: open list:DOCUMENTATION, Dwaipayan Ray, Joe Perches On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez <martin.fernandez@eclypsium.com> wrote: > > Add a description, an example and a possible workaround to the > MACRO_ARG_REUSE check. > > Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > --- > 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 11:29 ` Lukas Bulwahn @ 2022-07-06 13:26 ` Martin Fernandez 2022-07-06 14:21 ` Lukas Bulwahn 0 siblings, 1 reply; 11+ messages in thread From: Martin Fernandez @ 2022-07-06 13:26 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: open list:DOCUMENTATION, Dwaipayan Ray, Joe Perches On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez > <martin.fernandez@eclypsium.com> wrote: >> >> Add a description, an example and a possible workaround to the >> MACRO_ARG_REUSE check. >> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> >> --- >> 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 >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 13:26 ` Martin Fernandez @ 2022-07-06 14:21 ` Lukas Bulwahn 2022-07-06 17:19 ` Martin Fernandez 0 siblings, 1 reply; 11+ messages in thread From: Lukas Bulwahn @ 2022-07-06 14:21 UTC (permalink / raw) To: Martin Fernandez; +Cc: open list:DOCUMENTATION, Dwaipayan Ray, Joe Perches On Wed, Jul 6, 2022 at 3:26 PM Martin Fernandez <martin.fernandez@eclypsium.com> wrote: > > On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > > On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez > > <martin.fernandez@eclypsium.com> wrote: > >> > >> Add a description, an example and a possible workaround to the > >> MACRO_ARG_REUSE check. > >> > >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > >> --- > >> 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? 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 14:21 ` Lukas Bulwahn @ 2022-07-06 17:19 ` Martin Fernandez 0 siblings, 0 replies; 11+ messages in thread From: Martin Fernandez @ 2022-07-06 17:19 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: open list:DOCUMENTATION, Dwaipayan Ray, Joe Perches On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > On Wed, Jul 6, 2022 at 3:26 PM Martin Fernandez > <martin.fernandez@eclypsium.com> wrote: >> >> On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: >> > On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez >> > <martin.fernandez@eclypsium.com> wrote: >> >> >> >> Add a description, an example and a possible workaround to the >> >> MACRO_ARG_REUSE check. >> >> >> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> >> >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> >> >> --- >> >> 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-04 22:57 [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE Martin Fernandez 2022-07-06 7:30 ` Bagas Sanjaya 2022-07-06 11:29 ` Lukas Bulwahn @ 2022-07-06 15:09 ` Akira Yokosawa 2022-07-06 17:24 ` Martin Fernandez 2 siblings, 1 reply; 11+ messages in thread From: Akira Yokosawa @ 2022-07-06 15:09 UTC (permalink / raw) To: Martin Fernandez Cc: dwaipayanray1, joe, linux-doc, lukas.bulwahn, Akira Yokosawa Hi, Minor nit on reST syntax. On Mon, 4 Jul 2022 19:57:57 -0300, Martin Fernandez wrote: > Add a description, an example and a possible workaround to the > MACRO_ARG_REUSE check. > > Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> > Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > --- > 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**> + Using the same argument multiple times in the macro definition > + would lead to unwanted side-effects. You don't need manual emphasis as above, as this list is already in the form of so-called "Definition Lists" [1, 2]. [1]: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#definition-lists [2]: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lists-and-quote-like-blocks Defined terms will be automatically emphasized by Sphinx and should look in bold face in the generated HTML/PDF. (Style of emphasis might be customized by configuration.) It looks like there exists other similar patterns in this file (or might as well be in other related .rst files). I'd suggest removing those manual emphases in a follow-up patch. This is only a weak suggestion, and there is no urgency. Of course, if you have a reason to do the manual emphases, there is no need to change. Thanks, Akira > + > + 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. > + > + 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. > + > **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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE 2022-07-06 15:09 ` Akira Yokosawa @ 2022-07-06 17:24 ` Martin Fernandez 0 siblings, 0 replies; 11+ messages in thread From: Martin Fernandez @ 2022-07-06 17:24 UTC (permalink / raw) To: Akira Yokosawa; +Cc: dwaipayanray1, joe, linux-doc, lukas.bulwahn On 7/6/22, Akira Yokosawa <akiyks@gmail.com> wrote: > Hi, > Minor nit on reST syntax. > > On Mon, 4 Jul 2022 19:57:57 -0300, Martin Fernandez wrote: >> Add a description, an example and a possible workaround to the >> MACRO_ARG_REUSE check. >> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com> >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com> >> --- >> 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**> + Using the same argument multiple times in the macro >> definition >> + would lead to unwanted side-effects. > > You don't need manual emphasis as above, as this list is already > in the form of so-called "Definition Lists" [1, 2]. > > [1]: > https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#definition-lists > [2]: > https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lists-and-quote-like-blocks Thank you for the references. > Defined terms will be automatically emphasized by Sphinx and > should look in bold face in the generated HTML/PDF. > (Style of emphasis might be customized by configuration.) > > It looks like there exists other similar patterns in this file > (or might as well be in other related .rst files). I'd suggest > removing those manual emphases in a follow-up patch. > > This is only a weak suggestion, and there is no urgency. > Of course, if you have a reason to do the manual emphases, > there is no need to change. That's interesting. Didn't really know that. I just saw this unknown warning for me and since there were no documentation about it I decided to quickly add it using the rest of the document as a template. I agree that that's not a very good approach but it was very quick :) I'll consider checking the syntax of the whole document for further patches, thank you for the suggestion. > Thanks, Akira > >> + >> + 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. >> + >> + 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. >> + >> **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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-06 17:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-04 22:57 [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE Martin Fernandez 2022-07-06 7:30 ` Bagas Sanjaya 2022-07-06 13:19 ` Martin Fernandez 2022-07-06 14:03 ` Akira Yokosawa 2022-07-06 14:07 ` Martin Fernandez 2022-07-06 11:29 ` Lukas Bulwahn 2022-07-06 13:26 ` Martin Fernandez 2022-07-06 14:21 ` Lukas Bulwahn 2022-07-06 17:19 ` Martin Fernandez 2022-07-06 15:09 ` Akira Yokosawa 2022-07-06 17:24 ` Martin Fernandez
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.