On Fri, Apr 10, 2015 at 11:37 AM, Alan Conway <[email protected]> wrote:
> > On Wed, 2015-04-08 at 14:50 -0400, Andrew Stitcher wrote: > > On Wed, 2015-04-08 at 10:48 -0400, Alan Conway wrote: > > > ... > > > The issue is that we need buffer growth AND control over allocation. > > > pn_buffer_t forces use of malloc/free/realloc. That won't help if > you're > > > trying to get the data into a buffer allocated by Go for example. I > > > agree a growable buffer type is a nicer API than raw function pointers, > > > but the buffer type itself would need to use function pointers so we > can > > > replace the functions used for alloc/free/realloc. > > > > I think this is a pretty general issue actually that also crops up in > > embedded systems, where you want some different control over memory > > allocation. > > > > I think it might be better addressed by making malloc/free/realloc > > replaceable for the whole of proton - would that solve your go issue? > > Sadly no. A proton realloc surrogate is just going to be passed void* > and return void*. There's no memory safe way to implement that in Go, > unless someone keeps a Go pointer to the returned buffer it gets garbage > collected. There's no way to communicate a Go pointer thru a C proton > interface. > > Given all that, I think I prefer the sprintf-style solution for growing > buffers. It keeps the memory management strictly on the caller side of > the pn_ interface which is simpler. > > If there is a big pn_data overhaul in the works then maybe we should not > be talking about pn_data_encode and instead make sure we fix it in the > overhaul. > > I notice we have these other APIs: > > codec.h:499:PN_EXTERN int pn_data_format(pn_data_t *data, char *bytes, > size_t *size); > message.h:739:PN_EXTERN int pn_message_encode(pn_message_t *msg, char > *bytes, size_t *size); > ssl.h:320:PN_EXTERN int pn_ssl_get_peer_hostname( pn_ssl_t *ssl, char > *hostname, size_t *bufsize ); > > That is a better signature IMO, the new (backward compatible) syntax for > such functions would be: > > return 0: *size is updated to be the actual size. > return PN_OVERFLOW: *size is updated to be the required size. > return < 0: *size is undefined. > don't return > 0 > > Perhaps for the overhaul we should ensure all such functions (including > the replacement for pn_data_encode) follow this pattern? > I'm a big +1 on standardizing on a single snprintf style signature for all encode functions. The signatures you mention above (similar to sprintf but using an IN/OUT parameter) were my first attempt at a standard encode/decode signature. I agree with you that at first it looks a bit more appealing for some reason, however after using it for a while I actually found it quite cumbersome relative to the traditional snprintf style. In the common case it takes almost twice as much code to use it since you always have to declare, initialize, and check an extra variable in separate steps due to the IN/OUT parameter. It also doesn't map that nicely when you swig it since most languages don't do the IN/OUT parameter thing well, so you have to have more cumborsome/custom typemaps and/or wrapper code to deal with the impedance mismatch. For those reasons I propose that we standardize on the traditional snprintf rather than using the IN/OUT variant. --Rafael
