Re: [openpgp] [RFC4880bis PATCH] Clarify CRC-24 C example implementation

[Daniel Kahn Gillmor <[e-mail]>]

On Fri 2021-03-19 00:08:02 +0100, Ángel wrote:
> On 2021-03-18 at 09:49 +0100, Werner Koch wrote:
>> Anyway, I am in favor of applying Hal Finney's old suggestion (patch
>> 39, attached).
>
>
> As the goal is to provide clear code, that ORed 0x1000000 may still be
> a bit surprising (albeit less than when it was included in the
> constant). I think it would be clearer to use
>
>                    if (crc & 0x1000000) {
>                        crc = (crc ^ CRC24_POLY) & 0xffffff;
>                    }
>
>
> or, following more closely hal's second suggestion:
>                    if (crc & 0x1000000) {
>                        crc &= 0xffffff; /* Clear bit 25 to avoid overflow */
>                        crc ^= CRC24_POLY;
>                    }

fwiw, i'm fine with either of these.  I note that the change i'd
originally proposed renamed the constant to CRC24_GENERATOR, to align it
with the term in the text ("poly" doesn't appear anywhere in the text,
but "generator" does).

That said, we are pretty clearly in bike-shedding territory here.

I have heard no one advocate for leaving the text unchanged, and i have
heard no one advocate for just silently amending the code to drop bit 25
from the constant.

All of the other variants proposed in this thread seem to have people
expressing various degrees of mild preference, so long as:

 - bit 25 should be cleared during accumulation
 - the accumulator type is explicitly and clearly unsigned
 - the variable definition matches the constant in the text

i observe that the first two points are mutually redundant -- when the
accumulator is unsigned, then overflow doesn't hit undefined behavior,
if i'm reading the C spec correctly.  But it still seems cleaner and
easier to understand if the sample code includes both belt and
suspenders.

<chair hat on>
I think the editors should pick one particular color of bike shed that
satisfies the conditions above and include it in the next revision.
If a member of the WG strongly objects to the revision, we can have more
discussion, but otherwise, i think the WG should focus on more pressing
topics.
</chair hat off>

        --dkg