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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux