Re: [Cbor] 🔔 WGLC with request for reviews on cbor-network-addresses-05

[Carsten Bormann <[e-mail]>]

Hi Marco,

Thank you for your quick and detailed comments.

> [Section 1]
> 
> * The normative statement "Prefixes MUST omit ..." feels strange for the introduction, also as the recap of BCP14 terminology comes after that. I suggest to rather use "must omit" or even just "omit"; the intended normative text comes later on in Section 3.1 and 3.2 anyway.

I like the simple “omit”.

> [Section 3]
> 
> * Maybe some details can be made clearer in the third from last paragraph. For instance, it can be easier here to think only in terms of bit lengths. Proposed rephrasing
> 
>    "Then, taking whichever is smaller between the length in bits of the included byte-string and the prefix-length possibly rounded up to the next multiple of 8, copy that many bits from the byte-string into the array."

People won’t know how to copy bits.

So here is my merged text:

Then taking whichever is smaller between (a) the length of the
included byte-string, and (b) the number of bytes covered by the
prefix-length rounded up to the next multiple of 8: fail if that
number is greater than 16 (or 4), and then copy that many bytes from
the byte-string into the array.

Might be easier in pseudocode :-)

> * I guess "of 8 values" should be "with value 8" ?

Well, I’d not use a static array of 8 values here (and why let the readers guess about that array?), I’d simply use 0xFF << unused_bits:

unused_bits = (-prefix_length_in_bits) & 7;
if (length_in_bytes > 0)
  address_bytes[length_in_bytes - 1] &= (0xFF << unused_bits);

Or if you want to check:

if (length_in_bytes > 0 &&
    (address_bytes[length_in_bytes - 1] & ~(0xFF << unused_bits))
    != 0)
  fail();

Writing a test for this code is left as an exercise for the reader :-)

I’m not sure we even want to cover the masking out case; we should tell people to fail on incorrectly set bits (i.e., tag is invalid).

> == Nits ==

(Done unless mentioned:)

> In the second and third paragraph, I suggest to use only "document" rather than "specification", consistently with the rest of the text.

I went the other direction (document ➔ specification).

> s/set it/and then set it

I rephrased this and the next nit as above.

Now in https://github.com/cbor-wg/cbor-network-address/pull/5

Grüße, Carsten