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

[David Gibson]

On Sat, Aug 23, 2014 at 02:30:12PM +1000, Stuart Longland wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 23/08/14 11:40, David Gibson wrote:
> >>> 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.
> 
> Ahh okay, you learn something new every day then. :-)
> 
> I guess I don't trust errno in a threaded context because of its
> history.  Plus, I often code in C on platforms where it probably
> *isn't* threadsafe (i.e. bare-iron embedded).

Right, fair enough.

> >> 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.
> 
> This is doable.  The other thing I can do is do both: set errno *and*
> return it (negated if need be).

In fact, I was just thinking of suggesting that.  I think that's a
fairly good idea, though it would be nice to hear Rusty's view on this
as well.

[snip]
> >>>> +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
> 
> If I run ccanlint directly withot ccan/str listed in the dependencies,
> I get:
> > stringbuilder: Module's CCAN dependencies are the only CCAN files
> > #included (depends_accurate): FAIL 
> > /home/stuartl/hamradio/hhcp/ccan/ccan/stringbuilder/test/run.c:6:ccan/str
> > not listed in _info
> 
> But then if I comment out that line and run ccanlint again:
> > stringbuilder: Module's CCAN dependencies are the only CCAN files
> > #included (depends_accurate): PASS 
> > /home/stuartl/hamradio/hhcp/ccan/ccan/stringbuilder/_info:ccan/str
> > is an unused dependency
> 
> So the advice is contradictory.  What I did in the last patch was to
> make my own streq work-alike, ugly sure, but until I get ccanlint
> sorted, it works.  I guess a merger with str might solve that problem. :-)

Ah, I think you need to list separate testdepends, since ccan/str is
only being used in the tests, not the module proper here.  See
ccan/rfc822/_info amongst others for an example.

> Where ccanlint fails is running `make check-stringbuilder`:
> > tools/ccanlint/ccanlint --deps-fail-ignore ccan/build_assert 
> > build_assert: Total score: 32/32 make: Circular check-str <-
> > check-str dependency dropped. make: *** No rule to make target
> > `check-tal/talloc', needed by `check-str'.  Stop.

Ugh.. not sure what's up with that.

-- 
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/20140826/5fd05486/attachment.sig>