Subject: Re: [Cbor] 🔔 WGLC with request for reviews on cbor-cddl-control-03

From: Henk Birkholz <[email-masked]>
Date: Wed, 14 Jul 2021 13:27:31 +0200

Hi list,
hi Cristian,

now that past Henk has survived the cut-off, here is my review for 
cddl-control. I am way behind with all kind of review tasks, but as this 
has the most delay, this was first :-) Please find below the 
corresponding review. I think this memo is in an appropriate state to 
progress.

Viele Grüße,

Henk

I-D.ietf-cbor-cddl-control adds control operators to the CDDL via its 
intrinsic extension mechanism. The proposed additional controls have 
been discussed on the list and in several meetings and are deemed useful 
and appropriately framed.

While .plus, .cat and .det are pretty straight forward, .abnf and 
.feature are a bit more complex and therefore take up the majority of 
the body of the text.

The document is structured well and comes with excellent readability. 
The table up-front provides a concise overview and is providing the 
basis for the following expositional text and IANA Considerations. Table 
1 also indicates two variants of ABNF encoding that is supported. That 
simply could have been included in earlier text as .det is also listed 
next to .cat, but that is not a really a problem for the reader.

Numeric Addition is really helpful when curating interoperability with 
TLV'ish representations of data as these really often work with offsets. 
The example in Figure 1 is intuitive and works fine for me.

String Concatenation is pretty straight forward and hints at tiny 
implementation challenges that can be taken on with a bit of care.

The example for Dedenting concatenation of strings is the most important 
part for the implementer. It works well again and is intuitive. .det is 
an okay name. I would have also really been okay with .dedcat.

.det already prepares the reader for the embedded ABNF control .abnf. As 
some of the implication for implementers are not trivial, this section 
provides a chunk load of context info and details the issues. .abnfb is 
a bit hidden in the section, but as it is listed in the table up-front 
that is effectively not a problem.

NIT: It is highlighted in Section 2.3 how .det is useful to .abnfb, yet 
in Section 3 the context in the itemized list only illustrates .cat in 
the expositional text. The example shows how this all comes together, 
though, using quite intuitive type names. Maybe quickly reiterate the 
purpose of .det here in the text before the example so implementers that 
are looking that up finds all they need in the text itself.

Feature is written relatively abstract and was the hardest for me to 
find intuitive. The examples also take a bit more thinking. Feature is 
relative abstract by intent. As I would not know how to frame it better, 
this is a good solution.

NIT: To simplify the layout of the map in Figure 5, maybe a consistent 
use of "=>" instead of mixing ":" and "=>" might be easier for readers? 
Maybe it is okay to show that they can be mixed here, but might cause 
more confusion than necessary

NIT: I am not a huge fan of "remains to be seen" conjecture in RFC text, 
but well, it's true, so it is probably useful to readers.

On 01.06.21 20:34, Christian Amsüss wrote:
> Hello CBOR group,
> 
> this mail marks the start of a two week WGLC on cddl-control[1].
> 
> As this document has seen no recent review activity, I urge all
> prospective users to read the current version and provide reviews.
> 
> Without positive reviews, this WGLC can not complete.
> 
> Best regards,
> Christian
> 
> [1]: https://datatracker.ietf.org/doc/draft-ietf-cbor-cddl-control/
> 
> 
> _______________________________________________
> CBOR mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/cbor
>

---

From: Carsten Bormann <[email-masked]>
Date: Sat, 17 Jul 2021 21:02:37 +0200

Hi Henk,

Thank you for this review.

I’m preparing an updated version of cddl-control for post-WGLC processing.

> The document is structured well and comes with excellent readability. The table up-front provides a concise overview and is providing the basis for the following expositional text and IANA Considerations. Table 1 also indicates two variants of ABNF encoding that is supported. That simply could have been included in earlier text as .det is also listed next to .cat, but that is not a really a problem for the reader.

Table 1 is pretty early (like after the second sentence); is there another “earlier text” that should say this?
Section 3 does introduce both .abnf/.abnfb.

>  .det is an okay name. I would have also really been okay with .dedcat.

(There is a little note there why .det was ultimately chosen…)

> .det already prepares the reader for the embedded ABNF control .abnf. As some of the implication for implementers are not trivial, this section provides a chunk load of context info and details the issues. .abnfb is a bit hidden in the section, but as it is listed in the table up-front that is effectively not a problem.

Right, the flow of the exposition means that .abnfb is only introduced in earnest in the first bullet point.  Introducing more of its details earlier could increase confusion, so I kept the intro short.

> NIT: It is highlighted in Section 2.3 how .det is useful to .abnfb, yet in Section 3 the context in the itemized list only illustrates .cat in the expositional text. The example shows how this all comes together, though, using quite intuitive type names. Maybe quickly reiterate the purpose of .det here in the text before the example so implementers that are looking that up finds all they need in the text itself.

I actually interchanged the use .cat and .det in Figure 4, now that .det is both-handed.  A simpler example that is not employing RFC 5234 boilerplate would just use .det, but I’m not sure that another example really would add much.  I also added .det to the end of the last bullet item.

> Feature is written relatively abstract and was the hardest for me to find intuitive. The examples also take a bit more thinking. Feature is relative abstract by intent. As I would not know how to frame it better, this is a good solution.

Indeed, it is a very general tool, and a specification writer needs to make up their mind how to use it.  draft-ietf-asdf-sdf at least shows one way.

> NIT: To simplify the layout of the map in Figure 5, maybe a consistent use of "=>" instead of mixing ":" and "=>" might be easier for readers? Maybe it is okay to show that they can be mixed here, but might cause more confusion than necessary

They not only “can”, but the line 

  * (text .feature "further-person-extension") => any

really does need the =>
Of course, we could translate all the other
foo:
into
“foo” => 

but that would be a somewhat unnatural use of CDDL.

Specification writers that are at the stage where they need .feature probably should have mastered : vs. => already.

> NIT: I am not a huge fan of "remains to be seen" conjecture in RFC text, but well, it's true, so it is probably useful to readers.

I also think that indicating that there is room for the spec writer’s own thinking is a good direction to take here.

Changes, together with the ones based on my comments (which I’m not going to reply to about how much I agree with them :-) are in

https://github.com/cbor-wg/cddl-control/pull/7
https://cbor-wg.github.io/cddl-control/
(Weird, the latter should be the wglc branch besides the main branch.)

(Don’t forget to `gem update cddl` to play around with the both-handed .det.)

Grüße, Carsten