Re: [Gen-art] Genart last call review of draft-ietf-cbor-cddl-05

[Carsten Bormann <[e-mail]>]

Hi Ines,

Thank you very much for the review and the slight embarrassment it causes to the authors…

Comment 3 below we will pick up in the WG meeting today; I’ll report about that later.
The other comments are now addressed in the editor’s copy:

https://github.com/cbor-wg/cddl/commits/master
(Individual commits referenced below; the CI build is failing right now because of a weird connectivity issue.)

We plan to submit the fixes as part of -06 early next week.

Grüße, Carsten



> On Sep 29, 2018, at 05:02, Ines Robles <[e-mail]> wrote:
> 
> Reviewer: Ines Robles
> Review result: Ready with Nits
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-cbor-cddl-05
> Reviewer: Ines Robles
> Review Date: 2018-09-28
> IETF LC End Date: 2018-10-04
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary:
> 
> I believe the draft is technically good. This document is well written and
> clear to understand.
> 
> The document proposes a notational convention named Concise data definition
> language (CDDL) to express Concise Binary Object Representation (CBOR) data
> structures. Its main goal is to provide an easy and unambiguous way to express
> structures for protocol messages and data formats that use CBOR or JSON.
> 
> I have some minor questions. The document presents nits that should be solved
> before publication.
> 
> Major issues: Not found.
> 
> Minor issues: Not found.
> 
> Nits/editorial comments:
> 
> 1)-Abstract: It would be nice to expand CBOR the first time that it is
> mentioned.

Indeed.  Fixed in 88e9a00

> 2)-Section 2:  Page 8,  You mention anonymous group.
> Does it would be a good definition for an anonymous group?: “Lists of
> group_entries_  (between curly braces)that can be assigned to any type of
> composition (arrays, maps)”

The groups are anonymous because they are used right away in a pair of curly braces (for a map) or square brackets (for an array).
The examples in this section all happen to be in braces.
I’m not sure that “anonymous group” needs to be defined as a separate term, though, because it is just that: a group that is anonymous.

> 3)- Section 2.2.2.1 - Page 11: the ordering I think is ascendant, right? So,
> maybe, would it be nice to add “natural ordering relationship”? question: is
> the following valid?:
> 
>        Take-off = {
>                takeoffcounting= 10..0 ; Is it valid? Is it applicable?
> }

The type constituted by the range does not have any ordering relationship (except that implied by numbering in general).
The example you give would not make a difference from saying 0..10 — in both cases, the eleven numbers 0 to 10 constitute the type.

It is probably not so good that the current text seems to leave open whether 10..0 means the same as 0..10.

(There is no obvious right answer, so we will discuss this in the CBOR WG meeting today and come back.)

> 4)- In this paragraph: “...When using a name as the left hand side of a range
> operator, use spacing as in
>   "min .. max" to separate off the range operator...."
> => Question: Is “min .. max" equivalent to "min” .. “max"?

Sorry for leading the reader into quoting hell.
The quotes here are generated by XML2RFC to delimit a code span, i.e., they identify the example.
Maybe we should make small figures out of these (ignoring that some people don’t like sentences running over figures).

Fixed in a530564

> 
> 5)- There are some mismatches between Appendix B and Appendix C:
> 
>        Appendix B: cddl = S 1*(rule S)
>        Appendix C: cddl = S 1*rule
> ------------------------------------------------------------
> 
>        Appendix B:
> rule = typename [genericparm] S assignt S type
>         / groupname [genericparm] S assigng S grpent
> 
>        Appendix C:
>  rule = typename [genericparm] S assignt S type S
>        / groupname [genericparm] S assigng S grpent S
> ------------------------------------------------------------
> 
>        Appendix B:    type = type1 *(S "/" S type1)
>        Appendix C:    type = type1 S *("/" S type1 S)
> 
> ------------------------------------------------------------
>        Appendix B:   group = grpchoice *(S "//" S grpchoice)
>        Appendix C:   group = grpchoice S *("//" S grpchoice S)
> 
> ------------------------------------------------------------
>        Appendix B:    grpchoice = *(grpent optcom)
>        Appendix C:    grpchoice = *grpent
> 
> ------------------------------------------------------------
> 
>        Appendix B:    grpent = [occur S] [memberkey S] type
>        Appendix C:    grpent = [occur S] [memberkey S] type optcom
> 
> ------------------------------------------------------------
> 
>        Appendix B:    / [occur S] groupname [genericarg]  ; preempted by above
>        Appendix C:    / [occur S] groupname [genericarg] optcom ; preempted by
>        above
> 
> ------------------------------------------------------------
> 
>        Appendix B:   / [occur S] "(" S group S ")"
>        Appendix C:   / [occur S] “(" S group S ")" optcom

This is the embarrassing part.  We recently cleaned up the whitespace processing in the ABNF a bit, and I was sure all the changes made it to the matching appendix.  D’oh.  Another case of “a feature isn’t there if it isn’t checked by continuous integration checking”.

Fixed in bfdecd8

> 
> 6)- It would be nice to expand I-JSON to (“Internet JSON") in page 54

Fixed in cc43317

Grüße, Carsten