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

[David Gibson]

On Sat, Aug 23, 2014 at 09:28:20AM +1000, Stuart Longland wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 18/08/14 13:26, David Gibson wrote:
> > 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).
> 
> I did think about this too… I thought I'd play it safe by putting it
> by itself, with a view that it could be merged with the 'str' module
> if needed.

Ok.

> As to naming, I was thinking of "building up a string", since delim
> can be NULL indicating no delimiter, and then it's like repeatedly
> calling strcat, except it'll take care of how long the destination
> buffer is.
> 
> > [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.
> 
> A fair point, I'll look into this.
> 
> >> +{ +	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.
> 
> Ahh okay, my intent was to hint to the compiler that the function
> *should* be inlined since it'll be called a lot.

For a static function, a modern compiler can work that out better
than you can.

[snip]
> >> +/** + * 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.
> 
> My preference for doing the former is that it's more thread safe,
> something errno is definitely not.

So, errno is generally threadsafe these days, albeit via somewhat
tortured means.  Generally modern errno will be either a magic macro,
or (for compilers that support it) a thread local variable, which will
reference a per-thread errno value.

> I recognise that it's not conventional practice but if I'm going to
> return one thing for "all okay" and another for "error", then why not
> return the error code via a route where it's less likely to get
> splattered by another system call?

Hrm.  I'm torn.  You're right that returning the error code is a
better interface, but I'm not sure that's a strong enough reason to
break with convention.

At the very least I'd suggest returning -EWHATEVER, rather than
EWHATEVER.  I think when people see a function returning an integer
status, they're going to expect that (rc < 0) is a suitable check for
an error condition.

> >> + * + * 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.
> 
> Ahh okay, I hadn't thought of that option.  I'll do so.

Yeah, it's becoming a common ccan idiom.

[snip]
> >> + +/* + * Even though stringbuilder is implemented using + * 
> >> stringbuilder_va, we need to test it explicitly.
> > 
> > Really?  Why?
> 
> I honestly don't know, but it wasn't happy until I did so.
> Or maybe I was misinterpreting ccanlint.  It'll give a score, but then
> leaves me to figure out what I missed.
> 
> Perhaps I haven't fully figured out how to drive it yet. :-)

Hrm, that is very odd.  I wonder if Rusty has any ideas why that would
be.

> >> +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.
> 
> I did try this, but had all kinds of issues with getting the tests to
> compile.  I don't recall the exact errors I got.

This is also very strange

-- 
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/20140823/12da5db0/attachment-0001.sig>