Re: [PATCH RFT 1/5] bitops: add generic parity calculation for u8

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

Hi Rasmus,

thanks for the review!

> I know it's prototypical bikeshedding, but what's with the "get_"
> prefix? Certainly the purpose of any pure function is to "get" the
> result of some computation. We don't have "get_strlen()".

Hmmm, can be argued.

The reason I chose the naming was that the open coded implementations
were sometimes either returning the current parity or returning the bit
which you need to enforce a certain parity. And this was never obvious
from their names. Sometimes it was not even clear if the returned bit
was for odd or even parity, just for "their" parity whatever it was.

So, with "get_" I wanted to ensure that people can read from the name,
that this is about the current parity and one has to draw conclusions
from there. But maybe the kdoc documentation is good enough for this?

> Please either just "parity8", or if you want to emphasize that this
> belongs in the bit-munging family, "bit_parity8".

I'd go for 'parity8' then.

> > + *     if (get_parity8(val) == 0)
> > + *             val |= BIT(7);
> 
> Shouldn't that be ^= in general? Of course in some particular
> application one may have constructed the value in the lower 7 bits and
> "know" that bit 7 is currently clear.

Yeah, that is my use case. True, your example is more generic. Will fix.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]