On Wed, Sep 26, 2018 at 10:38 AM astitcher <g...@git.apache.org> 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.
