* [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses @ 2023-03-12 13:33 Menna Mahmoud 2023-03-12 14:12 ` Julia Lawall 0 siblings, 1 reply; 6+ messages in thread From: Menna Mahmoud @ 2023-03-12 13:33 UTC (permalink / raw) To: outreachy Cc: lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel, eng.mennamahmoud.mm enclose Macros with complex values in parentheses is especially useful in making macro definitions “safe” (so that they evaluate each operand exactly once). this error reported by chechpatch.pl "ERROR: Macros with complex values should be enclosed in parentheses" for ADE7854_SPI_SLOW, ADE7854_SPI_BURST and ADE7854_SPI_FAST macros and this error fixed by enclose these macros in parentheses. Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> --- drivers/staging/iio/meter/ade7854.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h index 7a49f8f1016f..41eeedef569b 100644 --- a/drivers/staging/iio/meter/ade7854.h +++ b/drivers/staging/iio/meter/ade7854.h @@ -139,9 +139,9 @@ #define ADE7854_MAX_RX 7 #define ADE7854_STARTUP_DELAY 1000 -#define ADE7854_SPI_SLOW (u32)(300 * 1000) -#define ADE7854_SPI_BURST (u32)(1000 * 1000) -#define ADE7854_SPI_FAST (u32)(2000 * 1000) +#define ADE7854_SPI_SLOW ((u32)(300 * 1000)) +#define ADE7854_SPI_BURST ((u32)(1000 * 1000)) +#define ADE7854_SPI_FAST ((u32)(2000 * 1000)) /** * struct ade7854_state - device instance specific data -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses 2023-03-12 13:33 [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses Menna Mahmoud @ 2023-03-12 14:12 ` Julia Lawall 2023-03-12 14:23 ` Menna Mahmoud 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2023-03-12 14:12 UTC (permalink / raw) To: Menna Mahmoud Cc: outreachy, lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2186 bytes --] On Sun, 12 Mar 2023, Menna Mahmoud wrote: > enclose Macros with complex values in parentheses is especially useful > in making macro definitions “safe” (so that they > evaluate each operand exactly once). enclose -> Enclose, and Macros -> macros I don't understand the above comment though. How does adding parentheses around the body of a macro cause the operands to be evaluated only once? And the macros that you have changed don't have any operands. The value of adding parentheses is normally to ensure that the body of the macro doesn't interact with the context in a weird way. For example, you could have #define ADD 3 + 4 Then if you use your macro as 6 * ADD, you will end up evaluating 6 * 3 + 4, ie 18 + 4, when you might have expected 6 * 7. The issue is that * has higher precedence than +. But I don't think that such a problem can arise with a cast expression, so parentheses around it should not be necessary. > this error reported by chechpatch.pl this error is reported by checkpatch. > > "ERROR: Macros with complex values should be enclosed in parentheses" > > for ADE7854_SPI_SLOW, ADE7854_SPI_BURST and ADE7854_SPI_FAST > macros and this error fixed by enclose these macros in parentheses. The last two lines aren't needed. One can easily see that from looking at the patch. julia > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > --- > drivers/staging/iio/meter/ade7854.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h > index 7a49f8f1016f..41eeedef569b 100644 > --- a/drivers/staging/iio/meter/ade7854.h > +++ b/drivers/staging/iio/meter/ade7854.h > @@ -139,9 +139,9 @@ > #define ADE7854_MAX_RX 7 > #define ADE7854_STARTUP_DELAY 1000 > > -#define ADE7854_SPI_SLOW (u32)(300 * 1000) > -#define ADE7854_SPI_BURST (u32)(1000 * 1000) > -#define ADE7854_SPI_FAST (u32)(2000 * 1000) > +#define ADE7854_SPI_SLOW ((u32)(300 * 1000)) > +#define ADE7854_SPI_BURST ((u32)(1000 * 1000)) > +#define ADE7854_SPI_FAST ((u32)(2000 * 1000)) > > /** > * struct ade7854_state - device instance specific data > -- > 2.34.1 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses 2023-03-12 14:12 ` Julia Lawall @ 2023-03-12 14:23 ` Menna Mahmoud 2023-03-12 14:25 ` Julia Lawall 0 siblings, 1 reply; 6+ messages in thread From: Menna Mahmoud @ 2023-03-12 14:23 UTC (permalink / raw) To: Julia Lawall Cc: outreachy, lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel On ١٢/٣/٢٠٢٣ ١٦:١٢, Julia Lawall wrote: > > On Sun, 12 Mar 2023, Menna Mahmoud wrote: > >> enclose Macros with complex values in parentheses is especially useful >> in making macro definitions “safe” (so that they >> evaluate each operand exactly once). > enclose -> Enclose, and Macros -> macros > > I don't understand the above comment though. How does adding parentheses > around the body of a macro cause the operands to be evaluated only once? > And the macros that you have changed don't have any operands. > > The value of adding parentheses is normally to ensure that the body of the > macro doesn't interact with the context in a weird way. For example, you > could have > > #define ADD 3 + 4 > > Then if you use your macro as 6 * ADD, you will end up evaluating > 6 * 3 + 4, ie 18 + 4, when you might have expected 6 * 7. The issue is > that * has higher precedence than +. yes, I mean that but i couldn't explain it well, thanks for your feedback. > > But I don't think that such a problem can arise with a cast expression, so > parentheses around it should not be necessary. So, no need for this patch? >> this error reported by chechpatch.pl > this error is reported by checkpatch. > >> "ERROR: Macros with complex values should be enclosed in parentheses" >> >> for ADE7854_SPI_SLOW, ADE7854_SPI_BURST and ADE7854_SPI_FAST >> macros and this error fixed by enclose these macros in parentheses. > The last two lines aren't needed. One can easily see that from looking at > the patch. Got it, Thank you. Menna > julia > >> Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> >> --- >> drivers/staging/iio/meter/ade7854.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h >> index 7a49f8f1016f..41eeedef569b 100644 >> --- a/drivers/staging/iio/meter/ade7854.h >> +++ b/drivers/staging/iio/meter/ade7854.h >> @@ -139,9 +139,9 @@ >> #define ADE7854_MAX_RX 7 >> #define ADE7854_STARTUP_DELAY 1000 >> >> -#define ADE7854_SPI_SLOW (u32)(300 * 1000) >> -#define ADE7854_SPI_BURST (u32)(1000 * 1000) >> -#define ADE7854_SPI_FAST (u32)(2000 * 1000) >> +#define ADE7854_SPI_SLOW ((u32)(300 * 1000)) >> +#define ADE7854_SPI_BURST ((u32)(1000 * 1000)) >> +#define ADE7854_SPI_FAST ((u32)(2000 * 1000)) >> >> /** >> * struct ade7854_state - device instance specific data >> -- >> 2.34.1 >> >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses 2023-03-12 14:23 ` Menna Mahmoud @ 2023-03-12 14:25 ` Julia Lawall 2023-03-15 0:25 ` Alison Schofield 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2023-03-12 14:25 UTC (permalink / raw) To: Menna Mahmoud Cc: Julia Lawall, outreachy, lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2722 bytes --] On Sun, 12 Mar 2023, Menna Mahmoud wrote: > > On ١٢/٣/٢٠٢٣ ١٦:١٢, Julia Lawall wrote: > > > > On Sun, 12 Mar 2023, Menna Mahmoud wrote: > > > > > enclose Macros with complex values in parentheses is especially useful > > > in making macro definitions “safe” (so that they > > > evaluate each operand exactly once). > > enclose -> Enclose, and Macros -> macros > > > > I don't understand the above comment though. How does adding parentheses > > around the body of a macro cause the operands to be evaluated only once? > > And the macros that you have changed don't have any operands. > > > > The value of adding parentheses is normally to ensure that the body of the > > macro doesn't interact with the context in a weird way. For example, you > > could have > > > > #define ADD 3 + 4 > > > > Then if you use your macro as 6 * ADD, you will end up evaluating > > 6 * 3 + 4, ie 18 + 4, when you might have expected 6 * 7. The issue is > > that * has higher precedence than +. > > > yes, I mean that but i couldn't explain it well, thanks for your feedback. > > > > > > But I don't think that such a problem can arise with a cast expression, so > > parentheses around it should not be necessary. > > > So, no need for this patch? No, I don't think so. julia > > > > > this error reported by chechpatch.pl > > this error is reported by checkpatch. > > > > > "ERROR: Macros with complex values should be enclosed in parentheses" > > > > > > for ADE7854_SPI_SLOW, ADE7854_SPI_BURST and ADE7854_SPI_FAST > > > macros and this error fixed by enclose these macros in parentheses. > > The last two lines aren't needed. One can easily see that from looking at > > the patch. > > > Got it, Thank you. > > Menna > > > julia > > > > > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > > > --- > > > drivers/staging/iio/meter/ade7854.h | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/staging/iio/meter/ade7854.h > > > b/drivers/staging/iio/meter/ade7854.h > > > index 7a49f8f1016f..41eeedef569b 100644 > > > --- a/drivers/staging/iio/meter/ade7854.h > > > +++ b/drivers/staging/iio/meter/ade7854.h > > > @@ -139,9 +139,9 @@ > > > #define ADE7854_MAX_RX 7 > > > #define ADE7854_STARTUP_DELAY 1000 > > > > > > -#define ADE7854_SPI_SLOW (u32)(300 * 1000) > > > -#define ADE7854_SPI_BURST (u32)(1000 * 1000) > > > -#define ADE7854_SPI_FAST (u32)(2000 * 1000) > > > +#define ADE7854_SPI_SLOW ((u32)(300 * 1000)) > > > +#define ADE7854_SPI_BURST ((u32)(1000 * 1000)) > > > +#define ADE7854_SPI_FAST ((u32)(2000 * 1000)) > > > > > > /** > > > * struct ade7854_state - device instance specific data > > > -- > > > 2.34.1 > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses 2023-03-12 14:25 ` Julia Lawall @ 2023-03-15 0:25 ` Alison Schofield 2023-03-15 4:19 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Alison Schofield @ 2023-03-15 0:25 UTC (permalink / raw) To: Julia Lawall, Dan Carpenter Cc: Menna Mahmoud, outreachy, lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel On Sun, Mar 12, 2023 at 03:25:37PM +0100, Julia Lawall wrote: > > > On Sun, 12 Mar 2023, Menna Mahmoud wrote: > > > > > On ١٢/٣/٢٠٢٣ ١٦:١٢, Julia Lawall wrote: > > > > > > On Sun, 12 Mar 2023, Menna Mahmoud wrote: > > > > > > > enclose Macros with complex values in parentheses is especially useful > > > > in making macro definitions “safe” (so that they > > > > evaluate each operand exactly once). > > > enclose -> Enclose, and Macros -> macros > > > > > > I don't understand the above comment though. How does adding parentheses > > > around the body of a macro cause the operands to be evaluated only once? > > > And the macros that you have changed don't have any operands. > > > > > > The value of adding parentheses is normally to ensure that the body of the > > > macro doesn't interact with the context in a weird way. For example, you > > > could have > > > > > > #define ADD 3 + 4 > > > > > > Then if you use your macro as 6 * ADD, you will end up evaluating > > > 6 * 3 + 4, ie 18 + 4, when you might have expected 6 * 7. The issue is > > > that * has higher precedence than +. > > > > > > yes, I mean that but i couldn't explain it well, thanks for your feedback. > > > > > > > > > > But I don't think that such a problem can arise with a cast expression, so > > > parentheses around it should not be necessary. > > > > > > So, no need for this patch? > > No, I don't think so. > > julia Looping in Dan C explicitly. Can we revisit this one? It actually leads to a checkpatch ERROR. So, anyone hoping to get an error free checkpatch run on this file, is out of luck. Is this something that checkpatch can learn about and allow, or should we add the parens here, to dare I say, appease the checkpatch god ;) Alison > > > > > > > > > this error reported by chechpatch.pl > > > this error is reported by checkpatch. > > > > > > > "ERROR: Macros with complex values should be enclosed in parentheses" > > > > > > > > for ADE7854_SPI_SLOW, ADE7854_SPI_BURST and ADE7854_SPI_FAST > > > > macros and this error fixed by enclose these macros in parentheses. > > > The last two lines aren't needed. One can easily see that from looking at > > > the patch. > > > > > > Got it, Thank you. > > > > Menna > > > > > julia > > > > > > > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com> > > > > --- > > > > drivers/staging/iio/meter/ade7854.h | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/staging/iio/meter/ade7854.h > > > > b/drivers/staging/iio/meter/ade7854.h > > > > index 7a49f8f1016f..41eeedef569b 100644 > > > > --- a/drivers/staging/iio/meter/ade7854.h > > > > +++ b/drivers/staging/iio/meter/ade7854.h > > > > @@ -139,9 +139,9 @@ > > > > #define ADE7854_MAX_RX 7 > > > > #define ADE7854_STARTUP_DELAY 1000 > > > > > > > > -#define ADE7854_SPI_SLOW (u32)(300 * 1000) > > > > -#define ADE7854_SPI_BURST (u32)(1000 * 1000) > > > > -#define ADE7854_SPI_FAST (u32)(2000 * 1000) > > > > +#define ADE7854_SPI_SLOW ((u32)(300 * 1000)) > > > > +#define ADE7854_SPI_BURST ((u32)(1000 * 1000)) > > > > +#define ADE7854_SPI_FAST ((u32)(2000 * 1000)) > > > > > > > > /** > > > > * struct ade7854_state - device instance specific data > > > > -- > > > > 2.34.1 > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses 2023-03-15 0:25 ` Alison Schofield @ 2023-03-15 4:19 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2023-03-15 4:19 UTC (permalink / raw) To: Alison Schofield Cc: Julia Lawall, Menna Mahmoud, outreachy, lars, Michael.Hennerich, jic23, gregkh, linux-iio, linux-staging, linux-kernel On Tue, Mar 14, 2023 at 05:25:06PM -0700, Alison Schofield wrote: > On Sun, Mar 12, 2023 at 03:25:37PM +0100, Julia Lawall wrote: > > > > > > On Sun, 12 Mar 2023, Menna Mahmoud wrote: > > > > > > > > On ١٢/٣/٢٠٢٣ ١٦:١٢, Julia Lawall wrote: > > > > > > > > On Sun, 12 Mar 2023, Menna Mahmoud wrote: > > > > > > > > > enclose Macros with complex values in parentheses is especially useful > > > > > in making macro definitions “safe” (so that they > > > > > evaluate each operand exactly once). > > > > enclose -> Enclose, and Macros -> macros > > > > > > > > I don't understand the above comment though. How does adding parentheses > > > > around the body of a macro cause the operands to be evaluated only once? > > > > And the macros that you have changed don't have any operands. > > > > > > > > The value of adding parentheses is normally to ensure that the body of the > > > > macro doesn't interact with the context in a weird way. For example, you > > > > could have > > > > > > > > #define ADD 3 + 4 > > > > > > > > Then if you use your macro as 6 * ADD, you will end up evaluating > > > > 6 * 3 + 4, ie 18 + 4, when you might have expected 6 * 7. The issue is > > > > that * has higher precedence than +. > > > > > > > > > yes, I mean that but i couldn't explain it well, thanks for your feedback. > > > > > > > > > > > > > > But I don't think that such a problem can arise with a cast expression, so > > > > parentheses around it should not be necessary. > > > > > > > > > So, no need for this patch? > > > > No, I don't think so. > > > > julia > > Looping in Dan C explicitly. > > Can we revisit this one? It actually leads to a checkpatch ERROR. > So, anyone hoping to get an error free checkpatch run on this file, > is out of luck. > > Is this something that checkpatch can learn about and allow, or > should we add the parens here, to dare I say, appease the checkpatch > god ;) > I think you wanted to CC Joe, not me? I agree with Julia, but I also have slightly kind of given up resisting on this one when people start adding unnecesary parentheses. Fixing the COMPLEX_MACRO macro warning to ignore cast operations would be a great idea for a small project. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-15 4:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-12 13:33 [PATCH] staging: iio: meter: enclose Macros with complex values in parentheses Menna Mahmoud 2023-03-12 14:12 ` Julia Lawall 2023-03-12 14:23 ` Menna Mahmoud 2023-03-12 14:25 ` Julia Lawall 2023-03-15 0:25 ` Alison Schofield 2023-03-15 4:19 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).