Hi Brane. It looks basically fine to me. A few stylistic comments below.
Branko Čibej wrote:
>> URL: http://svn.apache.org/viewvc?rev=1413482&view=rev
>> Log:
>> Factor the svn_stringbuf_t and svn_membuf_t implementation to a common
>> code base.
>>
>> * subversion/libsvn_subr/string.c (membuf_create, membuf_ensure): New,
>> private helper functions for membufs and stringbufs.
The 'r'-prefixed parameters designate 'return', I assume. In membuf_create() the output 'rpool' seems gratuitous: the caller could just as easily assign it as pass it.
membuf_create() has 'rdata' and 'rsize' are both plain outputs (makes sense), while membuf_ensure() has 'size' (in) and 'rsize' (in/out), and just one 'data' pointer (in/out). That all makes sense individually but I needed to inspect the code to figure it out. Please could the doc strings describe all the params, to be clear.
In the last few years be have become fairly consistent in putting output parameters first and pool params last. Would be good to stick to that.
>> (svn_membuf__create): Call membuf_create.
>> (svn_membuf__ensure): Call membuf_ensure.
>> (svn_membuf__resize): Call membuf_ensure for resizing the buffer.
>> (my__realloc): Remove.
>> (create_stringbuf): Remove. Inline implementation into the single call.
>> (svn_stringbuf_createv): Inline the previous contents of
> create_stringbuf.
>> (svn_stringbuf_create_ensure): Call membuf_create to allocate the buffer.
>> (svn_stringbuf_ensure): Call membuf_ensure to resize the buffer.
> Modified: subversion/trunk/subversion/libsvn_subr/string.c
> ==============================================================================
> +/* Allocate the space for a memory buffer. The data pointer will be NULL
> + * if the size is NULL.
> + * N.B.: The stringbuf creation functions use this, but since stringbufs
> + * stringbufs alwase consume at least 1 byte for the NUL terminator,
> + * the resulting data pointers will never be NULL.
> + */
> +static APR_INLINE void
> +membuf_create(apr_size_t size, apr_pool_t *pool,
> + void **rdata, apr_size_t *rsize, apr_pool_t **rpool)
> +{
> + /* apr_palloc will allocate multiples of 8.
> + * Thus, we would waste some of that memory if we stuck to the
> + * smaller size. Note that this is safe even if apr_palloc would
> + * use some other aligment or none at all. */
> + *rsize = (size ? APR_ALIGN_DEFAULT(size) : 0);
No need for the "?:" since APR_ALIGN_DEFAULT will round 0 up to the nearest multiple of 8 which is 0 anyway.
> + *rdata = (!*rsize ? NULL : apr_palloc(pool, *rsize));
> + if (rpool)
> + *rpool = pool;
> +}
> +
> +/* Ensure that the size of a given memory buffer is at least SIZE
> + * bytes. An existing buffer will be resized by multiplying its size
> + * by a power of two.
> + */
> +static APR_INLINE void
> +membuf_ensure(apr_size_t size, apr_pool_t *pool,
> + void **data, apr_size_t *rsize)
> +{
> + if (size > *rsize)
> + {
> + apr_size_t new_size = *rsize;
> +
> + if (new_size == 0)
> + /* APR will increase odd allocation sizes to the next
> + * multiple for 8, for instance. Take advantage of that
> + * knowledge and allow for the extra size to be used. */
> + new_size = APR_ALIGN_DEFAULT(size);
> + else
> + while (new_size < size)
> + {
> + /* new_size is aligned; doubling it should keep it aligned */
> + const apr_size_t prev_size = new_size;
'const' is typically not used on a local variable of short scope.
> + new_size *= 2;
> +
> + /* check for apr_size_t overflow */
> + if (prev_size > new_size)
> + {
> + new_size = APR_ALIGN_DEFAULT(size);
> + break;
> + }
> + }
> +
> + *data = (!new_size ? NULL : apr_palloc(pool, new_size));
> + *rsize = new_size;
> + }
> +}
> +
- Julian
Received on 2012-11-26 18:46:50 CET