Re: [Cbor] [Ext] Benjamin Kaduk's Discuss on draft-ietf-cbor-7049bis-14: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

Hi Paul,

On Fri, Sep 18, 2020 at 02:45:17PM +0000, Paul Hoffman wrote:
> Thank you for the extensive review! We'll respond to the DISCUSS parts of the
> ballot here; a later response will cover the COMMENTs.

Thanks; that seems a wise split.

> 
> On 2020-09-08, at 00:06, Benjamin Kaduk via Datatracker <[e-mail]> wrote:
> > 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-cbor-7049bis-14: Discuss
> > 
> > […]
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> > 
> > Thanks for this document; it's generally well-written and the changes
> > since 7049 are helpful.  I do have a few points that may need discussion
> > before publication, though.
> > 
> > Let's discuss whether the framing of tag number 35 for "regular
> > expressions that are roughly in [PCRE] form or a version of the
> > JavaScript regular expression syntax" meets the interoperability
> > expectations for Internet Standard status (bearing in mind that we are
> > defining a data format and not a protocol).  I note that it is okay
> > to leave the codepoint allocated with the current meaning and the
> > previous document as its reference, but decline to discuss it in the
> > document going for STD (we are in the process of doing that with COSE
> > countersignatures at the moment).
> 
> * — tag 35
> 
> Please see <https://github.com/cbor-wg/CBORbis/pull/210> for the proposed
> resolution. This PR is currently open.

That seems pretty much exactly like what I was expecting/hoping for;
thanks!

> > The example in Section 5 of "the item is a map that has byte strings for
> > keys and contains at least one pair whose key is 0xab01" seems to be in
> > violation of the definition of a valid map, since applications are not
> > allowed to rely on invalid behavior.  (That is, the implied "more than
> > one pair whose key is 0xab01" would be invalid.)
> 
> We have now changed this to "...and contains a pair whose key is 0xab01..." in
> <https://github.com/cbor-wg/CBORbis/pull/212>.

Thanks.

> > I think that the new deterministic encoding rules for sorting map keys
> > should be clear about whether "no content" sorts before or after
> > "content present" -- that is, how 0x10 and 0x1020 are ordered when the
> > 0x10 byte is identical and we have to compare \<nothing> with 0x20.
> 
> We have now added an implementation note at the end of 4.2.1
> in <https://github.com/cbor-wg/CBORbis/pull/212>.

Ah, a good point; sorry for missing that the first time around!

> > The discussion in Appendix C suggests that C (programming language)
> > implementations all use two's-complement representation of signed
> > integers; this requirement is present in POSIX but not C itself (I
> > verified this for C99 and C11).
> 
> Indeed. This no longer useful variation allowed by C finally seems to be fixed
> in draft C++ 20, which unanimously passed its final technical approval ballot
> on September 4, 2020: http://eel.is/c++draft/basic.fundamental
> 
> We agree the best way to handle this is to make this requirement explicit.
> C++20 is now referenced in new text in the part of the terminology section that
> talks about C-style notation in <https://github.com/cbor-wg/CBORbis/pull/208>.

This is probably fine, though using C++ as the reference for "notation
familiar from [C]" was a little surprising on first look.  (I actually had
missed that the draft C++20 was filing off this rough edge; that's good to
know.)

> > Additionally, the encode_sint() function (also Appendix C) relies on C
> > implementation-defined behavior while right-shifting a signed integer.
> 
> Similarly, this needless wart in C finally seems to be fixed in draft C++ 20 as
> well: http://eel.is/c++draft/expr.shift Also now referenced in 1.2
> in <https://github.com/cbor-wg/CBORbis/pull/208>.
> 
> > The C decode_half() function in Appendix D assumes that 'int' is wider
> > than 16 bits (since assigning a value to an int16_t variable when the
> > value is not representable in int16_t incurs implementation-defined
> > behavior).  Given that this spec is specifically targetting constrained
> > devices, it's not clear that such an assumption is justified.  (It also
> > right shifts a signed integer, incurring the same implementation-defined
> > behavior mentioned above.  (The bitwise AND against 0x8000 is also
> > problematic for an int16_t.))
> 
> Good point.  This can easily be fixed by declaring all the ints in that fragment as unsigneds instead.
> Done in <https://github.com/cbor-wg/CBORbis/pull/208>.

>From memory, that should work.  I will probably take another look at the
whole code when the revised I-D lands (but am under the weather at the
moment and don't have time to look right now).

Thanks for the updates!

-Ben