| Message ID | 20250306162541.2633025-2-visitorckw@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce and use generic parity16/32/64 helper | expand |
On Fri, 7 Mar 2025 00:25:26 +0800 Kuan-Wei Chiu <visitorckw@gmail.com> wrote: > Change return type to bool for better clarity. Update the kernel doc > comment accordingly, including fixing "@value" to "@val" and adjusting > examples. Also mark the function with __attribute_const__ to allow > potential compiler optimizations. If the result type is 'bool' you should just check it. Not compare against true/false. David > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > --- > include/linux/bitops.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index c1cb53cf2f0f..44e5765b8bec 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > /** > * parity8 - get the parity of an u8 value > - * @value: the value to be examined > + * @val: the value to be examined > * > * Determine the parity of the u8 argument. > * > * Returns: > - * 0 for even parity, 1 for odd parity > + * false for even parity, true for odd parity > * > * Note: This function informs you about the current parity. Example to bail > * out when parity is odd: > * > - * if (parity8(val) == 1) > + * if (parity8(val) == true) > * return -EBADMSG; > * > * If you need to calculate a parity bit, you need to draw the conclusion from > * this result yourself. Example to enforce odd parity, parity bit is bit 7: > * > - * if (parity8(val) == 0) > + * if (parity8(val) == false) > * val ^= BIT(7); > */ > -static inline int parity8(u8 val) > +static inline __attribute_const__ bool parity8(u8 val) > { > /* > * One explanation of this algorithm:
On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > Change return type to bool for better clarity. Update the kernel doc > comment accordingly, including fixing "@value" to "@val" and adjusting > examples. Also mark the function with __attribute_const__ to allow > potential compiler optimizations. > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > --- > include/linux/bitops.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index c1cb53cf2f0f..44e5765b8bec 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > /** > * parity8 - get the parity of an u8 value > - * @value: the value to be examined > + * @val: the value to be examined > * > * Determine the parity of the u8 argument. > * > * Returns: > - * 0 for even parity, 1 for odd parity > + * false for even parity, true for odd parity This occurs somehow inverted to me. When something is in parity means that it has equal number of 1s and 0s. I.e. return true for even distribution. Dunno what others think? Or perhaps this should be dubbed odd_parity() when bool is returned? Then you'd return true for odd. thanks,
* Jiri Slaby <jirislaby@kernel.org> wrote: > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > > Change return type to bool for better clarity. Update the kernel doc > > comment accordingly, including fixing "@value" to "@val" and adjusting > > examples. Also mark the function with __attribute_const__ to allow > > potential compiler optimizations. > > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > --- > > include/linux/bitops.h | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > > index c1cb53cf2f0f..44e5765b8bec 100644 > > --- a/include/linux/bitops.h > > +++ b/include/linux/bitops.h > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > /** > > * parity8 - get the parity of an u8 value > > - * @value: the value to be examined > > + * @val: the value to be examined > > * > > * Determine the parity of the u8 argument. > > * > > * Returns: > > - * 0 for even parity, 1 for odd parity > > + * false for even parity, true for odd parity > > This occurs somehow inverted to me. When something is in parity means that > it has equal number of 1s and 0s. I.e. return true for even distribution. > Dunno what others think? Or perhaps this should be dubbed odd_parity() when > bool is returned? Then you'd return true for odd. OTOH: - '0' is an even number and is returned for even parity, - '1' is an odd number and is returned for odd parity. Thanks, Ingo
On 07. 03. 25, 12:38, Ingo Molnar wrote: > > * Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >>> Change return type to bool for better clarity. Update the kernel doc >>> comment accordingly, including fixing "@value" to "@val" and adjusting >>> examples. Also mark the function with __attribute_const__ to allow >>> potential compiler optimizations. >>> >>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >>> --- >>> include/linux/bitops.h | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h >>> index c1cb53cf2f0f..44e5765b8bec 100644 >>> --- a/include/linux/bitops.h >>> +++ b/include/linux/bitops.h >>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >>> /** >>> * parity8 - get the parity of an u8 value >>> - * @value: the value to be examined >>> + * @val: the value to be examined >>> * >>> * Determine the parity of the u8 argument. >>> * >>> * Returns: >>> - * 0 for even parity, 1 for odd parity >>> + * false for even parity, true for odd parity >> >> This occurs somehow inverted to me. When something is in parity means that >> it has equal number of 1s and 0s. I.e. return true for even distribution. >> Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> bool is returned? Then you'd return true for odd. > > OTOH: > > - '0' is an even number and is returned for even parity, > - '1' is an odd number and is returned for odd parity. Yes, that used to make sense for me. For bool/true/false, it no longer does. But as I wrote, it might be only me... thanks,
* Jiri Slaby <jirislaby@kernel.org> wrote: > On 07. 03. 25, 12:38, Ingo Molnar wrote: > > > > * Jiri Slaby <jirislaby@kernel.org> wrote: > > > > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > > > > Change return type to bool for better clarity. Update the kernel doc > > > > comment accordingly, including fixing "@value" to "@val" and adjusting > > > > examples. Also mark the function with __attribute_const__ to allow > > > > potential compiler optimizations. > > > > > > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > > > --- > > > > include/linux/bitops.h | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > > > > index c1cb53cf2f0f..44e5765b8bec 100644 > > > > --- a/include/linux/bitops.h > > > > +++ b/include/linux/bitops.h > > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > > > /** > > > > * parity8 - get the parity of an u8 value > > > > - * @value: the value to be examined > > > > + * @val: the value to be examined > > > > * > > > > * Determine the parity of the u8 argument. > > > > * > > > > * Returns: > > > > - * 0 for even parity, 1 for odd parity > > > > + * false for even parity, true for odd parity > > > > > > This occurs somehow inverted to me. When something is in parity means that > > > it has equal number of 1s and 0s. I.e. return true for even distribution. > > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when > > > bool is returned? Then you'd return true for odd. > > > > OTOH: > > > > - '0' is an even number and is returned for even parity, > > - '1' is an odd number and is returned for odd parity. > > Yes, that used to make sense for me. For bool/true/false, it no longer does. > But as I wrote, it might be only me... No strong opinion on this from me either, I'd guess existing practice with other parity functions should probably control. (If a coherent praxis exists.). Thanks, Ingo
On March 7, 2025 4:13:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > >* Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >> > >> > * Jiri Slaby <jirislaby@kernel.org> wrote: >> > >> > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >> > > > Change return type to bool for better clarity. Update the kernel doc >> > > > comment accordingly, including fixing "@value" to "@val" and adjusting >> > > > examples. Also mark the function with __attribute_const__ to allow >> > > > potential compiler optimizations. >> > > > >> > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >> > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >> > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >> > > > --- >> > > > include/linux/bitops.h | 10 +++++----- >> > > > 1 file changed, 5 insertions(+), 5 deletions(-) >> > > > >> > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h >> > > > index c1cb53cf2f0f..44e5765b8bec 100644 >> > > > --- a/include/linux/bitops.h >> > > > +++ b/include/linux/bitops.h >> > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >> > > > /** >> > > > * parity8 - get the parity of an u8 value >> > > > - * @value: the value to be examined >> > > > + * @val: the value to be examined >> > > > * >> > > > * Determine the parity of the u8 argument. >> > > > * >> > > > * Returns: >> > > > - * 0 for even parity, 1 for odd parity >> > > > + * false for even parity, true for odd parity >> > > >> > > This occurs somehow inverted to me. When something is in parity means that >> > > it has equal number of 1s and 0s. I.e. return true for even distribution. >> > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> > > bool is returned? Then you'd return true for odd. >> > >> > OTOH: >> > >> > - '0' is an even number and is returned for even parity, >> > - '1' is an odd number and is returned for odd parity. >> >> Yes, that used to make sense for me. For bool/true/false, it no longer does. >> But as I wrote, it might be only me... > >No strong opinion on this from me either, I'd guess existing practice >with other parity functions should probably control. (If a coherent >praxis exists.). > >Thanks, > > Ingo Instead of "bool" think of it as "bit" and it makes more sense
On Fri, Mar 07, 2025 at 04:14:34AM -0800, H. Peter Anvin wrote: > On March 7, 2025 4:13:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > > > >* Jiri Slaby <jirislaby@kernel.org> wrote: > > > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: > >> > > >> > * Jiri Slaby <jirislaby@kernel.org> wrote: > >> > > >> > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > >> > > > Change return type to bool for better clarity. Update the kernel doc > >> > > > comment accordingly, including fixing "@value" to "@val" and adjusting > >> > > > examples. Also mark the function with __attribute_const__ to allow > >> > > > potential compiler optimizations. > >> > > > > >> > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > >> > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > >> > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > >> > > > --- > >> > > > include/linux/bitops.h | 10 +++++----- > >> > > > 1 file changed, 5 insertions(+), 5 deletions(-) > >> > > > > >> > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > >> > > > index c1cb53cf2f0f..44e5765b8bec 100644 > >> > > > --- a/include/linux/bitops.h > >> > > > +++ b/include/linux/bitops.h > >> > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > >> > > > /** > >> > > > * parity8 - get the parity of an u8 value > >> > > > - * @value: the value to be examined > >> > > > + * @val: the value to be examined > >> > > > * > >> > > > * Determine the parity of the u8 argument. > >> > > > * > >> > > > * Returns: > >> > > > - * 0 for even parity, 1 for odd parity > >> > > > + * false for even parity, true for odd parity > >> > > > >> > > This occurs somehow inverted to me. When something is in parity means that > >> > > it has equal number of 1s and 0s. I.e. return true for even distribution. > >> > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when > >> > > bool is returned? Then you'd return true for odd. > >> > > >> > OTOH: > >> > > >> > - '0' is an even number and is returned for even parity, > >> > - '1' is an odd number and is returned for odd parity. > >> > >> Yes, that used to make sense for me. For bool/true/false, it no longer does. > >> But as I wrote, it might be only me... > > > >No strong opinion on this from me either, I'd guess existing practice > >with other parity functions should probably control. (If a coherent > >praxis exists.). > > > >Thanks, > > > > Ingo > > Instead of "bool" think of it as "bit" and it makes more sense So, to help people thinking that way we can introduce a corresponding type: typedef unsigned _BitInt(1) u1; It already works for clang, and GCC is going to adopt it with std=c23. We can make u1 an alias to bool for GCC for a while. If you guys like it, I can send a patch. For clang it prints quite a nice overflow warning: tst.c:59:9: warning: implicit conversion from 'int' to 'u1' (aka 'unsigned _BitInt(1)') changes value from 2 to 0 [-Wconstant-conversion] 59 | u1 r = 2; | ~ ^ Thanks, Yury
On March 7, 2025 11:30:08 AM PST, Yury Norov <yury.norov@gmail.com> wrote: >On Fri, Mar 07, 2025 at 04:14:34AM -0800, H. Peter Anvin wrote: >> On March 7, 2025 4:13:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote: >> > >> >* Jiri Slaby <jirislaby@kernel.org> wrote: >> > >> >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >> >> > >> >> > * Jiri Slaby <jirislaby@kernel.org> wrote: >> >> > >> >> > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >> >> > > > Change return type to bool for better clarity. Update the kernel doc >> >> > > > comment accordingly, including fixing "@value" to "@val" and adjusting >> >> > > > examples. Also mark the function with __attribute_const__ to allow >> >> > > > potential compiler optimizations. >> >> > > > >> >> > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >> >> > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >> >> > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >> >> > > > --- >> >> > > > include/linux/bitops.h | 10 +++++----- >> >> > > > 1 file changed, 5 insertions(+), 5 deletions(-) >> >> > > > >> >> > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h >> >> > > > index c1cb53cf2f0f..44e5765b8bec 100644 >> >> > > > --- a/include/linux/bitops.h >> >> > > > +++ b/include/linux/bitops.h >> >> > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >> >> > > > /** >> >> > > > * parity8 - get the parity of an u8 value >> >> > > > - * @value: the value to be examined >> >> > > > + * @val: the value to be examined >> >> > > > * >> >> > > > * Determine the parity of the u8 argument. >> >> > > > * >> >> > > > * Returns: >> >> > > > - * 0 for even parity, 1 for odd parity >> >> > > > + * false for even parity, true for odd parity >> >> > > >> >> > > This occurs somehow inverted to me. When something is in parity means that >> >> > > it has equal number of 1s and 0s. I.e. return true for even distribution. >> >> > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> >> > > bool is returned? Then you'd return true for odd. >> >> > >> >> > OTOH: >> >> > >> >> > - '0' is an even number and is returned for even parity, >> >> > - '1' is an odd number and is returned for odd parity. >> >> >> >> Yes, that used to make sense for me. For bool/true/false, it no longer does. >> >> But as I wrote, it might be only me... >> > >> >No strong opinion on this from me either, I'd guess existing practice >> >with other parity functions should probably control. (If a coherent >> >praxis exists.). >> > >> >Thanks, >> > >> > Ingo >> >> Instead of "bool" think of it as "bit" and it makes more sense > >So, to help people thinking that way we can introduce a corresponding >type: > typedef unsigned _BitInt(1) u1; > >It already works for clang, and GCC is going to adopt it with std=c23. >We can make u1 an alias to bool for GCC for a while. If you guys like >it, I can send a patch. > >For clang it prints quite a nice overflow warning: > >tst.c:59:9: warning: implicit conversion from 'int' to 'u1' (aka 'unsigned _BitInt(1)') changes value from 2 to 0 [-Wconstant-conversion] > 59 | u1 r = 2; > | ~ ^ > >Thanks, >Yury No, for a whole bunch of reasons.
On Fri, 7 Mar 2025 12:42:41 +0100 Jiri Slaby <jirislaby@kernel.org> wrote: > On 07. 03. 25, 12:38, Ingo Molnar wrote: > > > > * Jiri Slaby <jirislaby@kernel.org> wrote: > > > >> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > >>> Change return type to bool for better clarity. Update the kernel doc > >>> comment accordingly, including fixing "@value" to "@val" and adjusting > >>> examples. Also mark the function with __attribute_const__ to allow > >>> potential compiler optimizations. > >>> > >>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > >>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > >>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > >>> --- > >>> include/linux/bitops.h | 10 +++++----- > >>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h > >>> index c1cb53cf2f0f..44e5765b8bec 100644 > >>> --- a/include/linux/bitops.h > >>> +++ b/include/linux/bitops.h > >>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > >>> /** > >>> * parity8 - get the parity of an u8 value > >>> - * @value: the value to be examined > >>> + * @val: the value to be examined > >>> * > >>> * Determine the parity of the u8 argument. > >>> * > >>> * Returns: > >>> - * 0 for even parity, 1 for odd parity > >>> + * false for even parity, true for odd parity > >> > >> This occurs somehow inverted to me. When something is in parity means that > >> it has equal number of 1s and 0s. I.e. return true for even distribution. > >> Dunno what others think? Or perhaps this should be dubbed odd_parity() when > >> bool is returned? Then you'd return true for odd. > > > > OTOH: > > > > - '0' is an even number and is returned for even parity, > > - '1' is an odd number and is returned for odd parity. > > Yes, that used to make sense for me. For bool/true/false, it no longer > does. But as I wrote, it might be only me... No me as well, I've made the same comment before. When reading code I don't want to have to look up a function definition. There is even scope for having parity_odd() and parity_even(). And, with the version that shifts a constant right you want to invert the constant! David
On March 7, 2025 11:36:43 AM PST, David Laight <david.laight.linux@gmail.com> wrote: >On Fri, 7 Mar 2025 12:42:41 +0100 >Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >> > >> > * Jiri Slaby <jirislaby@kernel.org> wrote: >> > >> >> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >> >>> Change return type to bool for better clarity. Update the kernel doc >> >>> comment accordingly, including fixing "@value" to "@val" and adjusting >> >>> examples. Also mark the function with __attribute_const__ to allow >> >>> potential compiler optimizations. >> >>> >> >>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >> >>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >> >>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >> >>> --- >> >>> include/linux/bitops.h | 10 +++++----- >> >>> 1 file changed, 5 insertions(+), 5 deletions(-) >> >>> >> >>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h >> >>> index c1cb53cf2f0f..44e5765b8bec 100644 >> >>> --- a/include/linux/bitops.h >> >>> +++ b/include/linux/bitops.h >> >>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >> >>> /** >> >>> * parity8 - get the parity of an u8 value >> >>> - * @value: the value to be examined >> >>> + * @val: the value to be examined >> >>> * >> >>> * Determine the parity of the u8 argument. >> >>> * >> >>> * Returns: >> >>> - * 0 for even parity, 1 for odd parity >> >>> + * false for even parity, true for odd parity >> >> >> >> This occurs somehow inverted to me. When something is in parity means that >> >> it has equal number of 1s and 0s. I.e. return true for even distribution. >> >> Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> >> bool is returned? Then you'd return true for odd. >> > >> > OTOH: >> > >> > - '0' is an even number and is returned for even parity, >> > - '1' is an odd number and is returned for odd parity. >> >> Yes, that used to make sense for me. For bool/true/false, it no longer >> does. But as I wrote, it might be only me... > >No me as well, I've made the same comment before. >When reading code I don't want to have to look up a function definition. >There is even scope for having parity_odd() and parity_even(). >And, with the version that shifts a constant right you want to invert >the constant! > > David > > > > Of course, for me, if I saw "parity_odd()" I would think of it as a function that caused the parity to become odd, i.e. if (!parity(x)) x ^= 1 << 7;
On 3/7/2025 11:36 AM, David Laight wrote: > On Fri, 7 Mar 2025 12:42:41 +0100 > Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >>> >>> * Jiri Slaby <jirislaby@kernel.org> wrote: >>> >>>> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >>>>> Change return type to bool for better clarity. Update the kernel doc >>>>> comment accordingly, including fixing "@value" to "@val" and adjusting >>>>> examples. Also mark the function with __attribute_const__ to allow >>>>> potential compiler optimizations. >>>>> >>>>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >>>>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >>>>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >>>>> --- >>>>> include/linux/bitops.h | 10 +++++----- >>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h >>>>> index c1cb53cf2f0f..44e5765b8bec 100644 >>>>> --- a/include/linux/bitops.h >>>>> +++ b/include/linux/bitops.h >>>>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >>>>> /** >>>>> * parity8 - get the parity of an u8 value >>>>> - * @value: the value to be examined >>>>> + * @val: the value to be examined >>>>> * >>>>> * Determine the parity of the u8 argument. >>>>> * >>>>> * Returns: >>>>> - * 0 for even parity, 1 for odd parity >>>>> + * false for even parity, true for odd parity >>>> >>>> This occurs somehow inverted to me. When something is in parity means that >>>> it has equal number of 1s and 0s. I.e. return true for even distribution. >>>> Dunno what others think? Or perhaps this should be dubbed odd_parity() when >>>> bool is returned? Then you'd return true for odd. >>> >>> OTOH: >>> >>> - '0' is an even number and is returned for even parity, >>> - '1' is an odd number and is returned for odd parity. >> >> Yes, that used to make sense for me. For bool/true/false, it no longer >> does. But as I wrote, it might be only me... > > No me as well, I've made the same comment before. > When reading code I don't want to have to look up a function definition. > There is even scope for having parity_odd() and parity_even(). > And, with the version that shifts a constant right you want to invert > the constant! > > David This is really a question of whether you expect odd or even parity as the "true" value. I think that would depend on context, and we may not reach a good consensus. I do agree that my brain would jump to "true is even, false is odd". However, I also agree returning the value as 0 for even and 1 for odd kind of made sense before, and updating this to be a bool and then requiring to switch all the callers is a bit obnoxious...
On March 12, 2025 4:56:31 PM PDT, Jacob Keller <jacob.e.keller@intel.com> wrote: > > >On 3/7/2025 11:36 AM, David Laight wrote: >> On Fri, 7 Mar 2025 12:42:41 +0100 >> Jiri Slaby <jirislaby@kernel.org> wrote: >> >>> On 07. 03. 25, 12:38, Ingo Molnar wrote: >>>> >>>> * Jiri Slaby <jirislaby@kernel.org> wrote: >>>> >>>>> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >>>>>> Change return type to bool for better clarity. Update the kernel doc >>>>>> comment accordingly, including fixing "@value" to "@val" and adjusting >>>>>> examples. Also mark the function with __attribute_const__ to allow >>>>>> potential compiler optimizations. >>>>>> >>>>>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >>>>>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >>>>>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >>>>>> --- >>>>>> include/linux/bitops.h | 10 +++++----- >>>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h >>>>>> index c1cb53cf2f0f..44e5765b8bec 100644 >>>>>> --- a/include/linux/bitops.h >>>>>> +++ b/include/linux/bitops.h >>>>>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >>>>>> /** >>>>>> * parity8 - get the parity of an u8 value >>>>>> - * @value: the value to be examined >>>>>> + * @val: the value to be examined >>>>>> * >>>>>> * Determine the parity of the u8 argument. >>>>>> * >>>>>> * Returns: >>>>>> - * 0 for even parity, 1 for odd parity >>>>>> + * false for even parity, true for odd parity >>>>> >>>>> This occurs somehow inverted to me. When something is in parity means that >>>>> it has equal number of 1s and 0s. I.e. return true for even distribution. >>>>> Dunno what others think? Or perhaps this should be dubbed odd_parity() when >>>>> bool is returned? Then you'd return true for odd. >>>> >>>> OTOH: >>>> >>>> - '0' is an even number and is returned for even parity, >>>> - '1' is an odd number and is returned for odd parity. >>> >>> Yes, that used to make sense for me. For bool/true/false, it no longer >>> does. But as I wrote, it might be only me... >> >> No me as well, I've made the same comment before. >> When reading code I don't want to have to look up a function definition. >> There is even scope for having parity_odd() and parity_even(). >> And, with the version that shifts a constant right you want to invert >> the constant! >> >> David > >This is really a question of whether you expect odd or even parity as >the "true" value. I think that would depend on context, and we may not >reach a good consensus. > >I do agree that my brain would jump to "true is even, false is odd". >However, I also agree returning the value as 0 for even and 1 for odd >kind of made sense before, and updating this to be a bool and then >requiring to switch all the callers is a bit obnoxious... Odd = 1 = true is the only same definition. It is a bitwise XOR, or sum mod 1.
On Wed, Mar 12, 2025 at 05:09:16PM -0700, H. Peter Anvin wrote: > On March 12, 2025 4:56:31 PM PDT, Jacob Keller <jacob.e.keller@intel.com> wrote: [...] > >This is really a question of whether you expect odd or even parity as > >the "true" value. I think that would depend on context, and we may not > >reach a good consensus. > > > >I do agree that my brain would jump to "true is even, false is odd". > >However, I also agree returning the value as 0 for even and 1 for odd > >kind of made sense before, and updating this to be a bool and then > >requiring to switch all the callers is a bit obnoxious... > > Odd = 1 = true is the only same definition. It is a bitwise XOR, or sum mod 1. The x86 implementation will be "popcnt(val) & 1", right? So if we choose to go with odd == false, we'll have to add an extra negation. So because it's a purely conventional thing, let's just pick a simpler one? Compiler's builtin parity() returns 1 for odd. Thanks, Yury
On Fri, Mar 07, 2025 at 11:33:40AM -0800, H. Peter Anvin wrote: > On March 7, 2025 11:30:08 AM PST, Yury Norov <yury.norov@gmail.com> wrote: [...] > >> Instead of "bool" think of it as "bit" and it makes more sense > > > >So, to help people thinking that way we can introduce a corresponding > >type: > > typedef unsigned _BitInt(1) u1; > > > >It already works for clang, and GCC is going to adopt it with std=c23. > >We can make u1 an alias to bool for GCC for a while. If you guys like > >it, I can send a patch. > > > >For clang it prints quite a nice overflow warning: > > > >tst.c:59:9: warning: implicit conversion from 'int' to 'u1' (aka 'unsigned _BitInt(1)') changes value from 2 to 0 [-Wconstant-conversion] > > 59 | u1 r = 2; > > | ~ ^ > > > >Thanks, > >Yury > > No, for a whole bunch of reasons. Can you please elaborate?
On March 13, 2025 9:24:38 AM PDT, Yury Norov <yury.norov@gmail.com> wrote: >On Wed, Mar 12, 2025 at 05:09:16PM -0700, H. Peter Anvin wrote: >> On March 12, 2025 4:56:31 PM PDT, Jacob Keller <jacob.e.keller@intel.com> wrote: > >[...] > >> >This is really a question of whether you expect odd or even parity as >> >the "true" value. I think that would depend on context, and we may not >> >reach a good consensus. >> > >> >I do agree that my brain would jump to "true is even, false is odd". >> >However, I also agree returning the value as 0 for even and 1 for odd >> >kind of made sense before, and updating this to be a bool and then >> >requiring to switch all the callers is a bit obnoxious... >> >> Odd = 1 = true is the only same definition. It is a bitwise XOR, or sum mod 1. > >The x86 implementation will be "popcnt(val) & 1", right? So if we >choose to go with odd == false, we'll have to add an extra negation. >So because it's a purely conventional thing, let's just pick a simpler >one? > >Compiler's builtin parity() returns 1 for odd. > >Thanks, >Yury The x86 implementation, no, but there will be plenty of others having that exact definition.
On 3/13/2025 9:36 AM, H. Peter Anvin wrote: > On March 13, 2025 9:24:38 AM PDT, Yury Norov <yury.norov@gmail.com> wrote: >> On Wed, Mar 12, 2025 at 05:09:16PM -0700, H. Peter Anvin wrote: >>> On March 12, 2025 4:56:31 PM PDT, Jacob Keller <jacob.e.keller@intel.com> wrote: >> >> [...] >> >>>> This is really a question of whether you expect odd or even parity as >>>> the "true" value. I think that would depend on context, and we may not >>>> reach a good consensus. >>>> >>>> I do agree that my brain would jump to "true is even, false is odd". >>>> However, I also agree returning the value as 0 for even and 1 for odd >>>> kind of made sense before, and updating this to be a bool and then >>>> requiring to switch all the callers is a bit obnoxious... >>> >>> Odd = 1 = true is the only same definition. It is a bitwise XOR, or sum mod 1. >> >> The x86 implementation will be "popcnt(val) & 1", right? So if we >> choose to go with odd == false, we'll have to add an extra negation. >> So because it's a purely conventional thing, let's just pick a simpler >> one? >> >> Compiler's builtin parity() returns 1 for odd. >> >> Thanks, >> Yury > > The x86 implementation, no, but there will be plenty of others having that exact definition. Makes sense to stick with that existing convention then. Enough to convince me. Thanks, Jake
On Thu, 13 Mar 2025 14:09:24 -0700 Jacob Keller <jacob.e.keller@intel.com> wrote: > On 3/13/2025 9:36 AM, H. Peter Anvin wrote: > > On March 13, 2025 9:24:38 AM PDT, Yury Norov <yury.norov@gmail.com> wrote: > >> On Wed, Mar 12, 2025 at 05:09:16PM -0700, H. Peter Anvin wrote: > >>> On March 12, 2025 4:56:31 PM PDT, Jacob Keller <jacob.e.keller@intel.com> wrote: > >> > >> [...] > >> > >>>> This is really a question of whether you expect odd or even parity as > >>>> the "true" value. I think that would depend on context, and we may not > >>>> reach a good consensus. > >>>> > >>>> I do agree that my brain would jump to "true is even, false is odd". > >>>> However, I also agree returning the value as 0 for even and 1 for odd > >>>> kind of made sense before, and updating this to be a bool and then > >>>> requiring to switch all the callers is a bit obnoxious... > >>> > >>> Odd = 1 = true is the only same definition. It is a bitwise XOR, or sum mod 1. > >> > >> The x86 implementation will be "popcnt(val) & 1", right? So if we > >> choose to go with odd == false, we'll have to add an extra negation. > >> So because it's a purely conventional thing, let's just pick a simpler > >> one? > >> > >> Compiler's builtin parity() returns 1 for odd. > >> > >> Thanks, > >> Yury > > > > The x86 implementation, no, but there will be plenty of others having that exact definition. > > Makes sense to stick with that existing convention then. Enough to > convince me. There is the possibility that the compiler will treat the builtin as having an 'int' result without the constraint of it being zero or one. In which case the conversion to bool will be a compare. This doesn't happen on x86-64 (gcc or clang) - but who knows elsewhere. For x86 popcnt(val) & 1 is best (except for parity8) but requires a non-archaic cpu. (Probably Nehelem or K10 or later - includes Sandy bridge and all the 'earth movers'.) Since performance isn't critical the generic C code is actually ok. (The 'parity' flag bit is only set on the low 8 bits.) David
On March 14, 2025 12:06:04 PM PDT, David Laight <david.laight.linux@gmail.com> wrote: >On Thu, 13 Mar 2025 14:09:24 -0700 >Jacob Keller <jacob.e.keller@intel.com> wrote: > >> On 3/13/2025 9:36 AM, H. Peter Anvin wrote: >> > On March 13, 2025 9:24:38 AM PDT, Yury Norov <yury.norov@gmail.com> wrote: >> >> On Wed, Mar 12, 2025 at 05:09:16PM -0700, H. Peter Anvin wrote: >> >>> On March 12, 2025 4:56:31 PM PDT, Jacob Keller <jacob.e.keller@intel.com> wrote: >> >> >> >> [...] >> >> >> >>>> This is really a question of whether you expect odd or even parity as >> >>>> the "true" value. I think that would depend on context, and we may not >> >>>> reach a good consensus. >> >>>> >> >>>> I do agree that my brain would jump to "true is even, false is odd". >> >>>> However, I also agree returning the value as 0 for even and 1 for odd >> >>>> kind of made sense before, and updating this to be a bool and then >> >>>> requiring to switch all the callers is a bit obnoxious... >> >>> >> >>> Odd = 1 = true is the only same definition. It is a bitwise XOR, or sum mod 1. >> >> >> >> The x86 implementation will be "popcnt(val) & 1", right? So if we >> >> choose to go with odd == false, we'll have to add an extra negation. >> >> So because it's a purely conventional thing, let's just pick a simpler >> >> one? >> >> >> >> Compiler's builtin parity() returns 1 for odd. >> >> >> >> Thanks, >> >> Yury >> > >> > The x86 implementation, no, but there will be plenty of others having that exact definition. >> >> Makes sense to stick with that existing convention then. Enough to >> convince me. > >There is the possibility that the compiler will treat the builtin as having >an 'int' result without the constraint of it being zero or one. >In which case the conversion to bool will be a compare. >This doesn't happen on x86-64 (gcc or clang) - but who knows elsewhere. > >For x86 popcnt(val) & 1 is best (except for parity8) but requires a non-archaic cpu. >(Probably Nehelem or K10 or later - includes Sandy bridge and all the 'earth movers'.) >Since performance isn't critical the generic C code is actually ok. >(The 'parity' flag bit is only set on the low 8 bits.) > > David > > You seem confused. We have already established that the built-in didn't currently produce good code on some cpus, but it does on others, with very little in between, so it would make sense to use the builtins on an opt-in basis. On x86 8- or 16-bit parity is best don't with test or xor respectively; 32- or 64-bit parity may use popcnt; test or by reducing down to a parity16 xor.
diff --git a/include/linux/bitops.h b/include/linux/bitops.h index c1cb53cf2f0f..44e5765b8bec 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) /** * parity8 - get the parity of an u8 value - * @value: the value to be examined + * @val: the value to be examined * * Determine the parity of the u8 argument. * * Returns: - * 0 for even parity, 1 for odd parity + * false for even parity, true for odd parity * * Note: This function informs you about the current parity. Example to bail * out when parity is odd: * - * if (parity8(val) == 1) + * if (parity8(val) == true) * return -EBADMSG; * * If you need to calculate a parity bit, you need to draw the conclusion from * this result yourself. Example to enforce odd parity, parity bit is bit 7: * - * if (parity8(val) == 0) + * if (parity8(val) == false) * val ^= BIT(7); */ -static inline int parity8(u8 val) +static inline __attribute_const__ bool parity8(u8 val) { /* * One explanation of this algorithm: