On Tue, Dec 17, 2013 at 11:35:54PM +0100, Daniel Martin wrote: > Signed-off-by: Daniel Martin <[email protected]> > --- > First patch and highly unrelated, but the next one adds tests here too. > And imo having tests doesn't hurt.
Reviewed-by: Peter Hutterer <[email protected]> with a few nitpicks below > > test/Makefile.am | 15 ++++++++++++++- > test/xstr.c | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+), 1 deletion(-) > create mode 100644 test/xstr.c > > diff --git a/test/Makefile.am b/test/Makefile.am > index 2852bb3..097b3a4 100644 > --- a/test/Makefile.am > +++ b/test/Makefile.am > @@ -5,7 +5,19 @@ if XORG > # Tests that require at least some DDX functions in order to fully link > # For now, requires xf86 ddx, could be adjusted to use another > SUBDIRS += xi2 > -noinst_PROGRAMS += xkb input xtest misc fixes xfree86 hashtabletest os > signal-logging touch > +noinst_PROGRAMS += \ > + fixes \ > + hashtabletest \ > + input \ > + misc \ > + os \ > + signal-logging \ > + touch \ > + xfree86 \ > + xkb \ > + xstr \ > + xtest > + > endif > check_LTLIBRARIES = libxservertest.la > > @@ -38,6 +50,7 @@ touch_LDADD=$(TEST_LDADD) > signal_logging_LDADD=$(TEST_LDADD) > hashtabletest_LDADD=$(TEST_LDADD) $(top_srcdir)/Xext/hashtable.c > os_LDADD=$(TEST_LDADD) > +xstr_LDADD=$(TEST_LDADD) > > libxservertest_la_LIBADD = $(XSERVER_LIBS) > if XORG > diff --git a/test/xstr.c b/test/xstr.c > new file mode 100644 > index 0000000..aba3e86 > --- /dev/null > +++ b/test/xstr.c > @@ -0,0 +1,36 @@ > +#ifdef HAVE_DIX_CONFIG_H > +#include <dix-config.h> > +#endif > + > +#include "misc.h" > + > +static void test_xstrtokenize(void) > +{ > + char tokenstr[] = "123|456|789"; > + char **tokens; > + > + tokens = (char **) xstrtokenize(tokenstr, "|"); you don't need the cast here > + > + assert(tokens); > + assert(*tokens); > + assert(*(tokens + 1)); > + assert(*(tokens + 2)); > + assert(*(tokens + 3) == NULL); > + > + assert(strcmp(*tokens, "123") == 0); > + assert(strcmp(*(tokens + 1), "456") == 0); > + assert(strcmp(*(tokens + 2), "789") == 0); I'd use tokens[0], tokens[1], etc, the *(tokens + 1) seems a bit convoluted. > + > + free(*tokens); > + free(*(tokens + 1)); > + free(*(tokens + 2)); > + free(tokens); > +} testing the easy case is good, but testing the odd cases is usually better. in this case: - behaviour for a zero-length token - behaviour for a zero-length string - behaviour for a string consisting only of tokens ("||||") - behaviour for a string mixed with strings and tokens ("a|b||c") these are the most likely cases to cause issues lateron, the standard use-case is usually well-tested anyway :) Cheers, Peter > + > +int > +main(int argc, char **argv) > +{ > + test_xstrtokenize(); > + > + return 0; > +} > -- > 1.8.5.1 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
