Re: svn commit: r1917047 - in /apr/apr/trunk: CHANGES buffer/ buffer/apr_buffer.c build.conf include/apr_buffer.h test/Makefile.in test/Makefile.win test/NWGNUaprtest test/abts_tests.h test/testbuffer.c test/testutil.h

On 17 Apr 2024, at 07:52, Ruediger Pluem <[email protected]> wrote:

>> +APR_DECLARE(apr_buffer_t *) apr_buffer_str_make(apr_pool_t *pool,
>> +                                                char *str, apr_ssize_t len)
>> +{
>> +    apr_buffer_t *buf;
>> +    apr_int64_t size;
>> +    apr_size_t slen;
>> +
>> +    if (!str) {
>> +        str = NULL;
>> +        size = 0;
>> +    }
>> +    if (APR_BUFFER_STRING == len && !str[0]) {
>> +        size = len;
>> +    }
> 
> Is the above just a performance shortcut?

It is, not sure if it's overkill.

>> +APR_DECLARE(apr_buffer_t *) apr_buffer_cpy(apr_buffer_t *dst,
>> +                                           const apr_buffer_t *src,
>> +                                           apr_buffer_alloc alloc, void *ctx)
>> +{
>> +    if (!src) {
>> +
>> +        dst->d.mem = NULL;
>> +        dst->size = 0;
>> +
>> +    }
>> +    else if (!alloc) {
>> +
>> +        dst->d.mem = src->d.mem;
>> +        dst->size = src->size;
>> +    
>> +    }
>> +    else {
>> +
>> +        /* absolute value is size of mem buffer including optional terminating zero */
>> +        apr_int64_t size = src->size < 0 ? -src->size : src->size;
>> +
>> +        void *mem = alloc(ctx, size);
>> +        memcpy(mem, src->d.mem, size);
>> +
>> +        dst->size = src->size;
>> +        dst->d.mem = mem;
> 
> Wouldn't it make sense to put the above 5 lines into a macro or a static inline function to reuse it
> here and in apr_buffer_arraydup?

My thoughts were to make all (most) of these macros, would that make sense?

> 
>> +
>> +    }
>> +
>> +    return dst;
>> +}
>> +
>> +
>> +APR_DECLARE(int) apr_buffer_cmp(const apr_buffer_t *src,
>> +                                const apr_buffer_t *dst)
>> +{
>> +    apr_size_t slen = apr_buffer_len(src);
>> +    apr_size_t dlen = apr_buffer_len(dst);
>> +   
>> +    if (slen != dlen) {
>> +        return slen < dlen ? -1 : 1;
>> +    }
>> +    else if (src->d.mem == dst->d.mem) {
>> +        /* identical data or both NULL */
>> +        return 0;
>> +    }
>> +    else if (!src->d.mem) {
>> +        return -1;
> 
> Can this case happen? We know that src->d.mem != dst->d.mem and if src->d.mem == NULL slen should be != dlen.
> 
>> +    }
>> +    else if (!dst->d.mem) {
>> +        return 1;
> 
> See above.

Let me look.

My brain is utterly fried looking at this (and the LDAP code).

>> +APR_DECLARE(char *) apr_buffer_pstrncat(apr_pool_t *p, const apr_buffer_t *buf,
>> +                                        int nelts, const char *sep, int flags,
>> +                                        apr_size_t *nbytes)
>> +{
>> +    const apr_buffer_t *src = buf;
>> +    apr_size_t seplen = sep ? strlen(sep) : 0;
>> +    apr_size_t size = 0;
>> +
>> +    char *dst, *str;
>> +
>> +    int i;
>> +    for (i = 0; i < nelts; i++) {
>> +
>> +        if (i > 0) {
>> +            size += seplen;
>> +        }
>> +
>> +        if (src->size < 0) {
>> +            size += (-src->size) - 1;
>> +        }
>> +        else {
>> +            if (APR_BUFFER_NONE == flags) {
>> +                size += src->size;
>> +            }
>> +            else if (APR_BUFFER_BASE64 == flags) {
>> +                apr_size_t b64len;
>> +
>> +                if (APR_SUCCESS != apr_encode_base64(NULL, src->d.mem, src->size,
>> +                                                     APR_ENCODE_NONE, &b64len)) {
>> +                    return NULL;
>> +                }
>> +                size += b64len - 1;
>> +            }
>> +        }
>> +
>> +        src++;
>> +    }
>> +
>> +    if (nbytes) {
>> +        *nbytes = size;
>> +    }
>> +
>> +    str = dst = apr_palloc(p, size + 1);
>> +
>> +    src = buf;
>> +
>> +    for (i = 0; i < nelts; i++) {
>> +
>> +        if (i > 0 && sep) {
>> +            strncpy(dst, sep, seplen);
>> +            dst += seplen;
>> +        }
>> +        
>> +        if (src->size < 0) {
>> +            strncpy(dst, src->d.str, (-src->size) - 1);
> 
> Why can't we use memcpy here? Do we assume that src->d.str contains a \0 prior to (-src->size) - 1?

We can.

Regards,
Graham
--