[ccan] [PATCH] stringbuilder: Functions for joining strings.

[David Gibson]

On Sun, Aug 17, 2014 at 09:16:05PM +1000, stuartl at longlandclan.yi.org wrote:
> From: Stuart Longland <stuartl at longlandclan.yi.org>
> 
> This is a small couple of functions for joining lists of strings
> together.  The lists can either be varadic arguments or arrays, and
> delimiters are optional.

So, I'm a bit torn as to whether this makes more sense as its own
module or an extension to the 'str' module.  I'd also suggest putting
"join" somewhere in the title, since that's the name the operation
you're performing often goes by (including in the tal/str module).

[snip]
> diff --git a/ccan/stringbuilder/stringbuilder.c b/ccan/stringbuilder/stringbuilder.c
> new file mode 100644
> index 0000000..cb2bc2d
> --- /dev/null
> +++ b/ccan/stringbuilder/stringbuilder.c
> @@ -0,0 +1,71 @@
> +/* CC0 (Public domain) - see LICENSE file for details */
> +#include <ccan/stringbuilder/stringbuilder.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +int stringbuilder(const char* delim, char* str, size_t str_sz, ...)

I'd suggest putting the destination arguments (str and str_sz) first
in all these functions, to match sprintf(), strcat() and others.

> +{
> +	int res;
> +	va_list ap;
> +	va_start(ap, str_sz);
> +	res = stringbuilder_va(delim, str, str_sz, ap);
> +	va_end(ap);
> +	return res;
> +}
> +
> +static inline int stringbuilder_cpy(
> +		char** str, size_t* str_sz, const char* s, size_t s_len)

Because this isn't in a header, you shouldn't specify "inline" here -
the compiler can work out for itself whether it's best to inline.

> +{
> +	if (*str != s) {
> +		if (!s_len)
> +			s_len = strlen(s);
> +		if (s_len > *str_sz)
> +			return EMSGSIZE;
> +		strcpy(*str, s);
> +	}
> +	*str += s_len;
> +	*str_sz -= s_len;
> +	return 0;
> +}
> +
> +int stringbuilder_va(const char* delim, char* str, size_t str_sz, va_list ap)
> +{
> +	int res = 0;
> +	size_t delim_len = 0;
> +	const char* s = va_arg(ap, const char*);
> +
> +	if (delim)
> +		delim_len = strlen(delim);
> +
> +	if (s)
> +		res = stringbuilder_cpy(&str, &str_sz, s, 0);
> +	while(s && !res) {
> +		s = va_arg(ap, const char*);

I think you can simplify the while/if logic here a bit, if you put the
va_arg assignment into the loop condition.

> +		if (s && delim)
> +			res = stringbuilder_cpy(&str, &str_sz,
> +					delim, delim_len);

And you can simplify it further, if you make stringbuilder_cpy treat
NULL as an empty string.

> +		if (s && !res)
> +			res = stringbuilder_cpy(&str, &str_sz,
> +						s, 0);
> +	}
> +	return res;
> +}
> +
> +int stringbuilder_array(const char* delim, char* str, size_t str_sz,
> +		const char** strings, size_t strings_sz)

I'd suggest renaming strings_sz to nstrings or something, since it's
not a bytelength like the other _sz parameters.  I'd also suggest
putting it before strings, since (argc, argv) establishes a pretty
strong precedent that the count goes first when dealing with
count/array pairs.

> +{
> +	int res = 0;
> +	size_t delim_len = 0;
> +
> +	if (delim)
> +		delim_len = strlen(delim);
> +
> +	while(strings_sz-- && !res) {
> +		res = stringbuilder_cpy(&str, &str_sz, *(strings++), 0);
> +		if (!res && delim)
> +			res = stringbuilder_cpy(&str, &str_sz,
> +					delim, delim_len);

IIUC correctly, if you just have  a single string in the array, this
will put a "delim" at the end, which I don't think is what you want,
and doesn't see to match the varargs variants.


> +	}
> +	return res;
> +
> +}
> diff --git a/ccan/stringbuilder/stringbuilder.h b/ccan/stringbuilder/stringbuilder.h
> new file mode 100644
> index 0000000..0d02dcb
> --- /dev/null
> +++ b/ccan/stringbuilder/stringbuilder.h
> @@ -0,0 +1,111 @@
> +/* CC0 (Public domain) - see LICENSE file for details */
> +#ifndef CCAN_STRINGBUILDER_H
> +#define CCAN_STRINGBUILDER_H
> +#include "config.h"
> +#include <stdarg.h>
> +#include <sys/types.h>
> +
> +/**
> + * stringbuilder - Join strings from a varadic list.  The list of arguments
> + * are all assumed to be of type const char* and must end with a NULL.  If the
> + * first argument is str, then the contents of str are preserved and appended
> + * to.
> + *
> + * @delim:	A delimiter to separate the strings with, or NULL.
> + * @str:	A pointer to a string buffer that will receive the result.
> + * @str_sz:	Size of the buffer pointed to by str.
> + *
> + * Returns:	0 on success
> + * 		EMSGSIZE if the resulting string would overflow the buffer.
> + * 		If an overflow condition is detected, the buffer content is
> + * 		NOT defined.

Returning an errno code is not a normal error interface.  Usually
you'd return -1 (or something negative), and set errno to EMSGSIZE.

> + *
> + * Example:
> + * 	int res;
> + * 	char file_name[80];
> + * 	res = stringbuilder("/", file_name, sizeof(file_name),
> + * 		"/var/lib/foo", "bar", "baz",
> + * 		NULL);
> + * 	if (res)
> + * 		printf("Failed to determine file name: %s",
> + * 			strerror(res));
> + * 	else
> + * 		printf("File is at %s", file_name);
> + */
> +int stringbuilder(const char* delim, char* str, size_t str_sz, ...);

It would be nice to use a macro wrapper around this to avoid having to
explicitly include the terminating NULL.

> +
> +/**
> + * stringbuilder_va - Join strings from a varadic list.  The list of arguments
> + * are all assumed to be of type const char* and must end with a NULL.  If the
> + * first argument is str, then the contents of str are preserved and appended
> + * to.
> + *
> + * @delim:	A delimiter to separate the strings with, or NULL.
> + * @str:	A pointer to a string buffer that will receive the result.
> + * @str_sz:	Size of the buffer pointed to by str.
> + *
> + * Returns:	0 on success
> + * 		EMSGSIZE if the resulting string would overflow the buffer.
> + * 		If an overflow condition is detected, the buffer content is
> + * 		NOT defined.
> + *
> + * Example:
> + * 	#include <ccan/stringbuilder/stringbuilder.h>
> + * 	#include <stdarg.h>
> + * 	#include <stdio.h>
> + * 	#include <string.h>
> + * 	#include <errno.h>
> + *
> + *	int my_stringbuilder(const char* delim, char* str,
> + *		size_t str_sz, ...);
> + *
> + *	int my_stringbuilder(const char* delim, char* str,
> + *		size_t str_sz, ...)
> + *	{


Somewhat contrived example, but ok..

> + *		int res;
> + *		va_list ap;
> + *		va_start(ap, str_sz);
> + *		res = stringbuilder_va(delim, str, str_sz, ap);
> + *		va_end(ap);
> + *		return res;
> + *	}
> + *
> + *	int main(void) {
> + *		char my_string[80];
> + *		int res = my_stringbuilder(" ", my_string,
> + *			sizeof(my_string), "foo", "bar", NULL);
> + *		if (!res)
> + *			printf("%s\n", my_string);
> + *		return res ? 1 : 0;
> + *	}
> + */
> +int stringbuilder_va(const char* delim, char* str, size_t str_sz, va_list ap);
> +
> +/**
> + * stringbuilder_array - Join strings from an array of const char* pointers.
> + *
> + * @delim:	A delimiter to separate the strings with, or NULL.
> + * @str:	A pointer to a string buffer that will receive the result.
> + * @str_sz:	Size of the buffer pointed to by str.
> + * @strings:	The array of strings to join.
> + * @strings_sz:	The number of strings to join.
> + *
> + * Returns:	0 on success
> + * 		EMSGSIZE if the resulting string would overflow the buffer.
> + * 		If an overflow condition is detected, the buffer content is
> + * 		NOT defined.
> + *
> + * Example:
> + * 	char my_args[128];
> + * 	int res = stringbuilder_array(", ", my_args, sizeof(my_args),
> + * 		(const char**)argv, argc);
> + * 	if (res)
> + * 		printf("Failed to list arguments: %s",
> + * 			strerror(res));
> + * 	else
> + * 		printf("My arguments were %s", my_args);
> + */
> +int stringbuilder_array(const char* delim, char* str, size_t str_sz,
> +		const char** strings, size_t strings_sz);
> +
> +#endif /* CCAN_STRINGBUILDER_H */
> diff --git a/ccan/stringbuilder/test/run.c b/ccan/stringbuilder/test/run.c
> new file mode 100644
> index 0000000..08d61a0
> --- /dev/null
> +++ b/ccan/stringbuilder/test/run.c
> @@ -0,0 +1,68 @@
> +#include <ccan/stringbuilder/stringbuilder.h>
> +#include <ccan/stringbuilder/stringbuilder.c>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <ccan/tap/tap.h>
> +
> +/* 
> + * Even though stringbuilder is implemented using
> + * stringbuilder_va, we need to test it explicitly.

Really?  Why?

> + */
> +int test_stringbuilder_va(const char* delim, char* str, size_t str_sz, ...);
> +
> +int test_stringbuilder_va(const char* delim, char* str, size_t str_sz, ...)
> +{
> +	int res;
> +	va_list ap;
> +	va_start(ap, str_sz);
> +	res = stringbuilder_va(delim, str, str_sz, ap);
> +	va_end(ap);
> +	return res;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char string[20];
> +	const char* str_array[] = {
> +		"xxx", "yyy"
> +	};
> +	int res;
> +
> +	res = test_stringbuilder_va(NULL, string, sizeof(string),
> +			"aaa", "bbb", NULL);
> +	printf("res: %s, string: %s\n",
> +			strerror(res), string);
> +	ok1(res == 0);
> +	ok1(!strcmp(string, "aaabbb"));

I suggest using streq() from ccan/str, rather than the potentially
confusing !strcmp construction.

> +
> +	res = stringbuilder(NULL, string, sizeof(string),
> +			"aaa", "bbb", NULL);
> +	printf("res: %s, string: %s\n",
> +			strerror(res), string);
> +	ok1(res == 0);
> +	ok1(!strcmp(string, "aaabbb"));
> +
> +	res = stringbuilder(NULL, string, sizeof(string),
> +			"aaaaa", "bbbbb", "ccccc", "ddddd",
> +			"eeeee", "fffff", NULL);
> +	printf("res: %s, string: %s\n",
> +			strerror(res), string);
> +	ok1(res == EMSGSIZE);
> +
> +	res = stringbuilder(", ", string, sizeof(string),
> +			"aaa", "bbb", NULL);
> +	printf("res: %s, string: %s\n",
> +			strerror(res), string);
> +	ok1(res == 0);
> +	ok1(!strcmp(string, "aaa, bbb"));
> +
> +	res = stringbuilder_array(NULL, string, sizeof(string),
> +			str_array, sizeof(str_array)/sizeof(str_array[0]));
> +	printf("res: %s, string: %s\n",
> +			strerror(res), string);
> +	ok1(res == 0);
> +	ok1(!strcmp(string, "xxxyyy"));

You don't have a testcase for stringbuilder_array with a delimiter,
which I think would find a bug as noted above.

> +
> +	return exit_status();
> +}				

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20140818/85911342/attachment.sig>