Re: [GitHub] qpid-proton pull request #159: PROTON-1940: [c] normalize encoding of multip...
On Wed, Sep 26, 2018 at 10:38 AM astitcher wrote: > > What is 'multiple="true" field'? There is nothing called this in the > standard is there? Alas there is, which is where the problem started. It is *yet another* way to represent a variable-length sequence of identically typed values but with new and redundant encoding options! Here are the proper links, I'll update the code: http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#doc-idp115568 http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#section-composite-type-representation I'll write this up properly for the next go-round The ="true' naming seems to indicate boolean fields, but this is not the > case afaict - and this really applies to a list of things that can also be > empty in which case we just use null instead of the empty list. > Correct - multiple="true" is part of the XML field definition, which is part of a composite definition, which is how frames and other stuff are described. A field with multiple="true" is allowed to be any of: null, empty array, single value, non-empty array. null and empty array MUST be treated as equivalent, so must single value and array of length one. All this redundancy means that some popular AMQP implementations (.NET) don't properly handle the empty-array case. Now technically that is their bug, but normalizing the encoding is more efficient anyway, and more likely to interop generally. Even multiple="false" fields are allowed to be null, so it's unlikely that anybody will choke on a null field. Also url to local file system is not very helpful! > Doh!
Re: [GitHub] qpid-proton pull request #159: PROTON-1940: [c] normalize encoding of multip...
On Wed, Sep 26, 2018 at 10:38 AM astitcher wrote: > Github user astitcher commented on a diff in the pull request: > > https://github.com/apache/qpid-proton/pull/159#discussion_r220587685 > > --- Diff: c/src/core/codec.c --- > @@ -529,15 +529,15 @@ int pn_data_vfill(pn_data_t *data, const char > *fmt, va_list ap) > case 'd': >err = pn_data_put_double(data, va_arg(ap, double)); >break; > -case 'Z': > +case 'Z': /* encode binary, must not be NULL */ > --- End diff -- > > As much as I agree the format letters need documenting I think that > doing it inline is *not* the right way to do it - IMO a single block > comment above would be much easier to encompass all the different codes. > The inline comments are for the benefit of someone (like me) who is trying to figure out what the heck vfill does. I'll leave them, but add a block comment at the top. I don't think we should put this in user doc, it's marked INTERNAL in the API and I don't think we should encourage its use as IMO it stinks. I's very hard to read the format strings and it is very easy to make a mistake. I think it would be much better to have some sequence of functions, macros or enums with short-but-meaningful names to identify types, rather than this crazy mini-language of chars. It is vaguely printf-like, but the codes don't follow any conventions that any C programmer is likely to understand, so I think emulating printf has little/no value.
