Interest in variadic pack/unpack functions?

[Graeme Smecher]

Hi,

I stumbled across jansson relatively recently, and I'm completely
thrilled. It's a clean and well-build project. Thanks!

I've whipped up a couple of variadic functions inspired by Python's
Py_BuildObject that greatly simplify the library's use in my code. I'd
love to see something like this included in the codebase, and I'm
willing to follow up if maintainers are interested. The two functions
are json_pack() and json_unpack(). Here's some motivation:

json_t *error = json_pack("{k:s,k:s}",
"source", "some_source",
"message", "a helpful message follows");

=> { "source": "some_source", "message": "a helpful message
follows" }

char *source, *message;
json_unpack(error, "{k:s,k:s}", &source, &message);
printf("Source: '%s', message '%s'\n", source, message);

=> "Source: 'some_source', message 'a helpful message follows'"

...or,

json_t *result = json_pack("[fs]", 3.5, "Volts")
"source", "some_source",
"message", "a helpful message follows");

=> [ 3.5, "Volts" ]

double d, char *u;
json_unpack(result, "[fs]", &d, &u);
printf("Value %f, units '%s'\n");

=> "Value 3.5, units 'Volts'"

(Example code neglects error handling, of course.)

Source code for the functions themselves follows. It's still a work-in-
progress; errors in the formatting string are handled via assertions
for the moment.

cheers,
Graeme Smecher

---

json_t *json_pack(const char *fmt, ...) {
va_list ap;

/* Keep a stack of containers (lists and objects) */
int depth = 0;
json_t **stack;

json_t *cur = NULL; /* Current container */
json_t *root = NULL; /* root object */
json_t *obj = NULL;

char *key = NULL; /* Current key in an object */

/* Allocation provisioned for worst case */
if(!((stack = calloc(strlen(fmt), sizeof(json_t *)))))
return(NULL);

va_start(ap, fmt);
while(*fmt) {
switch(*fmt) {
case ',': /* Element spacer */
assert(root);
assert(cur);
assert(json_is_array(cur) || json_is_object(cur));
assert(!key);
break;

case ':': /* Key/value separator */
assert(key); /* Got a key */
assert(json_is_object(cur));
break;

case '[':
case '{':
/* Build the object */
obj = *fmt=='[' ? json_array() : json_object();

/* Root it */
if(json_is_object(cur)) {
assert(key);
json_object_set_new(cur, key, obj);
key = NULL;
} else if(json_is_array(cur)) {
json_array_append_new(cur, obj);
} else if(!root) {
root = obj;
} else {
assert(0);
}

/* Descend */
stack[depth++] = cur;
cur = obj;

break;


case ']':
assert(!key);
assert(depth > 0);
assert(json_is_array(cur));

cur = stack[--depth];

break;

case '}':
assert(!key);
assert(depth > 0);
assert(json_is_object(cur));

cur = stack[--depth];

break;

case 'k': /* key string */
assert(!key);
assert(json_is_object(cur));
key = va_arg(ap, char*);
break;

case 's': /* string */
obj = json_string(va_arg(ap, char*));
goto regular_obj;

case 'b': /* boolean */
obj = va_arg(ap, int) ?
json_true() : json_false();
goto regular_obj;

case 'i': /* integer */
obj = json_integer(va_arg(ap, int));
goto regular_obj;

case 'f': /* double-precision float */
obj = json_real(va_arg(ap, double));
goto regular_obj;

case 'O': /* a json_t object; increments refcount */
obj = va_arg(ap, json_t *);
json_incref(obj);
goto regular_obj;

case 'o': /* a json_t object; doesn't increment refcount */
obj = va_arg(ap, json_t *);
goto regular_obj;

regular_obj:
if(json_is_object(cur)) {
assert(key);
json_object_set_new(cur, key, obj);
key = NULL;
} else if(json_is_array(cur)) {
json_array_append_new(cur, obj);
} else if(!root) {
root = obj;
} else {
assert(0);
}
break;
}
fmt++;
}
va_end(ap);

free(stack);

return(root);
}

int json_unpack(json_t *root, const char *fmt, ...) {
va_list ap;

int rv=0; /* Return value */

/* Keep a stack of containers (lists and objects) */
int depth = 0;
json_t **stack;
int *index_stack;

int array_index = 0;
char *key = NULL; /* Current key in an object */

json_t *cur = NULL; /* Current container */
json_t *obj = NULL;

/* Allocation provisioned for worst case */
stack = calloc(strlen(fmt), sizeof(json_t *));
index_stack = calloc(strlen(fmt), sizeof(int));
if(!stack || !index_stack) {
if(stack) free(stack);
if(index_stack) free(index_stack);
return(-1);
}

/* Even if we're successful, we need to know if the number of
* arguments provided matches the number of JSON objects.
* We can do this by counting the elements in every array or
* object we open up, and decrementing the count as we visit
* their children. */
int unvisited = 0;

va_start(ap, fmt);
while(*fmt) {
switch(*fmt) {
case ',': /* Element spacer */
assert(cur);
assert(json_is_array(cur) || json_is_object(cur));
assert(!key);
break;

case ':': /* Key/value separator */
assert(key); /* Got a key */
assert(json_is_object(cur));
break;

case '[':
case '{':
/* Fetch object */
if(!cur) {
obj = root;
} else if(json_is_object(cur)) {
assert(key);
obj = json_object_get(cur, key);
unvisited--;
key = NULL;
} else if(json_is_array(cur)) {
obj = json_array_get(cur, array_index);
unvisited--;
array_index++;
} else {
assert(0);
}

/* Make sure we got what we expected */
if(*fmt=='{' && !json_is_object(obj)) {
rv = -2;
goto out;
}
if(*fmt=='[' && !json_is_array(obj)) {
rv = -3;
goto out;
}

unvisited += json_is_object(obj) ?
json_object_size(obj) :
json_array_size(obj);

/* Descend */
stack[depth++] = cur;
cur = obj;

key = NULL;

break;

case ']':
assert(!key);
assert(depth > 0);
assert(json_is_array(cur));

cur = stack[--depth];

break;

case '}':
assert(!key);
assert(depth > 0);
assert(json_is_object(cur));

cur = stack[--depth];

break;

case 'k': /* constant string for key */
assert(!key);
if(!json_is_object(cur)) {
rv = -1;
goto out;
}
key = va_arg(ap, char*);
break;

case 's': /* string; borrowed reference */
case 'S': /* string; memcpy'd where you like */
case 'i': /* integer */
case 'f': /* double-precision float */
case 'O': /* a json_t object; increments refcount */
case 'o': /* a json_t object; borrowed reference */
case 'b': /* boolean */

/* Fetch object */
if(!cur) {
obj = root;
} else if(json_is_object(cur)) {
assert(key);
obj = json_object_get(cur, key);
unvisited--;
key = NULL;
} else if(json_is_array(cur)) {
obj = json_array_get(cur, array_index);
unvisited--;
array_index++;
} else {
assert(0);
}

switch(*fmt) {
case 's':
case 'S':
if(!json_is_string(obj)) {
rv = -4;
goto out;
}
if(*fmt == 'S')
strcpy(va_arg(ap, char*), json_string_value(obj));
else
*va_arg(ap, const char**) = json_string_value(obj);
break;

case 'i':
if(!json_is_integer(obj)) {
rv = -5;
goto out;
}
*va_arg(ap, int*) = json_integer_value(obj);
break;

case 'b':
if(!json_is_boolean(obj)) {
rv = -6;
goto out;
}
*va_arg(ap, int*) = json_is_true(obj);
break;

case 'f':
if(!json_is_number(obj)) {
rv = -7;
goto out;
}
*va_arg(ap, double*) = json_number_value(obj);
break;

case 'O':
json_incref(obj);
/* Fall through */

case 'o':
*va_arg(ap, json_t**) = obj;
break;

default: assert(0);
}
}
fmt++;
}

/* Return 0 if everything was matched; otherwise the number of JSON
* objects we didn't get to. */
rv = unvisited;

out:
va_end(ap);

if(stack)
free(stack);
if(index_stack)
free(index_stack);
return(rv);
}

[Petri Lehtinen]

Graeme Smecher wrote:
> I stumbled across jansson relatively recently, and I'm completely
> thrilled. It's a clean and well-build project. Thanks!

You're welcome. I'm happy to hear that someone shares my views of what
an API for handling JSON should look like :)

> I've whipped up a couple of variadic functions inspired by Python's
> Py_BuildObject that greatly simplify the library's use in my code. I'd
> love to see something like this included in the codebase, and I'm
> willing to follow up if maintainers are interested. The two functions
> are json_pack() and json_unpack(). Here's some motivation:

I think this is a great idea! After trying a while, I realized that I
can't find the right words to describe how thrilled I am about this :)

See my comments below.

>
> json_t *error = json_pack("{k:s,k:s}",
> "source", "some_source",
> "message", "a helpful message follows");
>
> => { "source": "some_source", "message": "a helpful message
> follows" }

> char *source, *message;
> json_unpack(error, "{k:s,k:s}", &source, &message);
> printf("Source: '%s', message '%s'\n", source, message);
>
> => "Source: 'some_source', message 'a helpful message follows'"

Did you mean this:

json_unpack(obj, "{k:s,k:s}", "source", &source, "message", &message);

As otherwise I don't see how json_unpack() would know which key to get
from the object. Your code also suggests the example should be like this.

> ...or,
>
> json_t *result = json_pack("[fs]", 3.5, "Volts")
> "source", "some_source",
> "message", "a helpful message follows");
>
> => [ 3.5, "Volts" ]

This has an obvious copy paste error, otherwise it's clear to me :)

>
> double d, char *u;
> json_unpack(result, "[fs]", &d, &u);
> printf("Value %f, units '%s'\n");
>
> => "Value 3.5, units 'Volts'"
>
> (Example code neglects error handling, of course.)
>
> Source code for the functions themselves follows. It's still a work-in-
> progress; errors in the formatting string are handled via assertions
> for the moment.

Your code looks good in general, although I would like to see some
changes:

- Error reporting. Assertions won't do, and we should put some thought
on how and with what detail we should return error information to
the user.

- There shouldn't be a separate format character for object keys. With
a bit of logic, 's' can be used, like this:

json_pack("{s:f}", "pi", 3.141592);

==> {"pi": 3.141592}

- Whitespace should be allowed inside format strings. This should be
easy to implement.

In addition to these, there's a bigger problem with your code. For
example, if I read the code correctly, this succeeds:

json_pack("{k:{k:s", "foo", "bar", "baz");

The result is the JSON object {"foo": {"bar: "baz"}} even though
closing curly brackets aren't there in the format string.

Checking for this (and maybe some other things) in both json_pack()
and json_unpack() is cumbersome and error prone. I started writing a
counter example for why the unvisited counter doesn't work, but hey, I
think you got it right! I wouldn't have :)

So, I suggest you use recursive functions instead of the explicit
stack. As you already noticed, there's not even a way of knowing the
size of the stack beforehand. Using a recursive parser, you don't have
to. The stack is implicit in the function calls, and it's far easier
to save anything you need in the stack (local variables in the
functions), not only the object you're currently parsing. The
unvisited counter, for example.

So, if you can do the suggested changes (or have a good counter
arguments against them), I'm more than willing to merge the code in.
I'd also like to see some tests...

Petri

[Petri Lehtinen]

Petri Lehtinen wrote:

> Graeme Smecher wrote:
> > I've whipped up a couple of variadic functions inspired by Python's
> > Py_BuildObject that greatly simplify the library's use in my code. I'd
> > love to see something like this included in the codebase, and I'm
> > willing to follow up if maintainers are interested. The two functions
> > are json_pack() and json_unpack(). Here's some motivation:

I thought of a few more functions that could be handy shortcuts for
simple one-level objects and arrays:

json_pack_object("iii", "foo", 1, "bar", 2 "baz", 3);

json_unpack_object("iii", "foo", &one, "bar", &two, "baz", &three);

json_pack_array("iii", 1, 2, 3);

json_unpack_array("iii", &one, &two, &three);

These functions would make simple cases more easy for the user,
especially the _object ones, as the format string syntax is
simplified.

Thoughts?

Petri

[Graeme Smecher]

Hi Peti,

On Sat, Oct 23, 2010 at 11:54 AM, Petri Lehtinen <pe...@digip.org> wrote:

Did you mean this:

 json_unpack(obj, "{k:s,k:s}", "source", &source, "message", &message);

As otherwise I don't see how json_unpack() would know which key to get
from the object. Your code also suggests the example should be like this.

Yup, of course. That's what I get for writing a quick e-mail on a Saturday when I should be doing other things. Ditto for the errors I've snipped from the reply. Sorry about that...

Your code looks good in general, although I would like to see some
changes:

- Error reporting. Assertions won't do, and we should put some thought
 on how and with what detail we should return error information to
 the user.

I agree. I think the following convention makes the most sense:

    0: unqualified success; one-to-one correspondence between JSON object and variadic arguments.
    >0: success, but some mismatch occurred (e.g. unvisited counter was nonzero)
    <0: error

However, we have two classes of error: programming errors (i.e. problems with the formatting string) and matching errors (i.e. the supplied JSON didn't conform to expectations.) I think both can return <0; programming errors shouldn't take long to hunt down if the function call never succeeds.

 

- There shouldn't be a separate format character for object keys. With
 a bit of logic, 's' can be used, like this:

     json_pack("{s:f}", "pi", 3.141592);

 ==> {"pi": 3.141592}

I had originally coded things this way. It's mostly a matter of taste, but using "s" for both constant strings (char *) and string arguments (char **) during json_unpack() calls seemed a little overloaded. Having changed the code for keys "k", though, I keep writing keys as "s" anyways and realizing my error. I don't feel strongly about it.

 

- Whitespace should be allowed inside format strings. This should be
 easy to implement.

I agree. This should only take a line or two, similar to the optional separators ',' and ':'.

 

In addition to these, there's a bigger problem with your code. For
example, if I read the code correctly, this succeeds:

   json_pack("{k:{k:s", "foo", "bar", "baz");

The result is the JSON object {"foo": {"bar: "baz"}} even though
closing curly brackets aren't there in the format string.

Checking for this (and maybe some other things) in both json_pack()
and json_unpack() is cumbersome and error prone. I started writing a
counter example for why the unvisited counter doesn't work, but hey, I
think you got it right! I wouldn't have :)

Correct, of course. Actually, you could argue that missing closures aren't a problem, since the results correspond to what the caller intended (even if they missed a bracket or two.) Checking for this is not a problem in a nonrecursive implementation; at the end of the formatting string, just ensure the stack depth is 0.

So, I suggest you use recursive functions instead of the explicit
stack. As you already noticed, there's not even a way of knowing the
size of the stack beforehand. Using a recursive parser, you don't have
to. The stack is implicit in the function calls, and it's far easier
to save anything you need in the stack (local variables in the
functions), not only the object you're currently parsing. The
unvisited counter, for example.

So, if you can do the suggested changes (or have a good counter
arguments against them), I'm more than willing to merge the code in.
I'd also like to see some tests...

Tests, yes! I had some, but I destroyed them by accident (and foolishly assumed I could whip together some flawless example code in an e-mail.)

I think my next step is to rework my code and resubmit it as a complete addition to the jansson tree, instead of a couple of functions in isolation. I will probably work on this over the next couple of days. In the meantime, let's get the rough edges in the API smoothed out.

cheers,

Graeme

[Deron Meranda]

On Mon, Oct 25, 2010 at 8:51 AM, Petri Lehtinen <pe...@digip.org> wrote:
> I thought of a few more functions that could be handy shortcuts for
> simple one-level objects and arrays:
>
>  json_pack_object("iii", "foo", 1, "bar", 2 "baz", 3);
>
>  json_unpack_object("iii", "foo", &one, "bar", &two, "baz", &three);
>
>  json_pack_array("iii", 1, 2, 3);
>
>  json_unpack_array("iii", &one, &two, &three);

Yes, those seem quite useful. May I suggest that for the packing
functions there be extend/append variants as well:

json_pack_object_extend( existingobj, "iii", "qux", 4, "quux", 5,
"corge", 6 );

json_pack_array_append( existingarr, "iii", 4, 5, 6 );

The object extend would replace existing key values.
Somebody can pick better function names if you want.
--
Deron Meranda
http://deron.meranda.us/

[Petri Lehtinen]

These sound useful to me, too. But, as the original value is given as
the first argument, we could get away with one function:

json_pack_extend(existing_obj_or_array, "iii", ...)

The rest depends on whether "existing_obj_or_array" is array or
object. The function name is analogous to json_object_extend and
json_array_extend, which both do exactly the same thing as the new
function would do (without the packing part, of course).

Having just one function may be a bit confusing, but at least I like
the function name more than json_pack_object_extend or
json_pack_array_append/extend.

Petri

[Petri Lehtinen]

Graeme Smecher wrote:
> - Error reporting. Assertions won't do, and we should put some thought
> on how and with what detail we should return error information to
> the user.
>
> I agree. I think the following convention makes the most sense:
>
> 0: unqualified success; one-to-one correspondence between JSON object and
> variadic arguments.
> >0: success, but some mismatch occurred (e.g. unvisited counter was
> nonzero)
> <0: error
>
> However, we have two classes of error: programming errors (i.e. problems with
> the formatting string) and matching errors (i.e. the supplied JSON didn't
> conform to expectations.) I think both can return <0; programming errors
> shouldn't take long to hunt down if the function call never succeeds.

I wasn't thinking about return values only.

What's really, and I mean REALLY, cool about json_unpack() is that it
can be used to conveniently extract data out of JSON arrays and
objects and to validate that they are correct, all with one call.

When something goes wrong with the validation part (i.e. a matching
error occurs), I think it's very important for the user to be able to
know exactly what went wrong. So how does this sound like:

json_error_t error;

if(json_unpack(value, "[i, f]", &intval, &doubleval, &error)) {
/* an error occured */
fprintf(stderr, "%d: %s\n", error.line, error.text);
}

and alternatively

if(json_unpack(value, "[i, f]", &intval, &doubleval, NULL)) {
/* an error occured, no error information returned */
}

The return value can be e.g. -1 for matching errors and -2 for format
string errors, but in both cases we could return an error message. The
line could be the line *in the format string*, on which the
mismatching format character was found.

And more food for thought: To make json_unpack() much more powerful
for data extraction, it should be possible to match only a part of an
array or, more importantly, an object. For this reason, a "..." format
specifier could be supported:

json_unpack(value, "{s: i, s: f, ...}", "foo", &i, "bar", &d, NULL);

This would allow the matching to succeed even if there are more keys
than just "foo" and "bar" in the object. For arrays this, would work
by matching only the first few values and not caring about the rest.

> - There shouldn't be a separate format character for object keys. With
> a bit of logic, 's' can be used, like this:
>
> json_pack("{s:f}", "pi", 3.141592);
>
> ==> {"pi": 3.141592}
>
>
> I had originally coded things this way. It's mostly a matter of taste, but
> using "s" for both constant strings (char *) and string arguments (char **)
> during json_unpack() calls seemed a little overloaded. Having changed the code
> for keys "k", though, I keep writing keys as "s" anyways and realizing my
> error. I don't feel strongly about it.

I think it should be "s" for keys. If starts to feel confusing at some
point, it's easy to allow "k" for keys as well... Or drop the
requirement to specify a format character for object keys at all, see
below.

> - Whitespace should be allowed inside format strings. This should be
> easy to implement.
>
> I agree. This should only take a line or two, similar to the optional
> separators ',' and ':'.

I agree that in arrays the "," separator could be optional. But in
objects, I don't feel that "{ssss}" or even "{ksks}" would be a good
option. I would either require the separators or remove the need for
specifying a format character for object keys, as the keys are always
strings anyway. In this case, the above example would just be "{ss}".

I had a quick peek at Python's C API documentation. It seems that they
impement the latter case, i.e. there's no separate format character
for object keys. And in Python, you can have any (hashable) value as a
dict key, not only strings.

I'm looking forward to see this in the Jansson tree, but I'm not sure
if this email smoothened the API or added more rough edges :)

Petri

[gsme...@gmail.com]

From: Graeme Smecher <graeme....@mail.mcgill.ca>

---
src/Makefile.am | 3 +-
src/jansson.h | 18 +-
src/jansson_private.h | 7 +
src/load.c | 7 -
src/variadic.c | 589 +++++++++++++++++++++++++++++++++++++++++
test/.gitignore | 2 +
test/suites/api/Makefile.am | 5 +-
test/suites/api/check-exports | 2 +
test/suites/api/test_pack.c | 155 +++++++++++
test/suites/api/test_unpack.c | 110 ++++++++
10 files changed, 881 insertions(+), 17 deletions(-)
create mode 100644 src/variadic.c
create mode 100644 test/suites/api/test_pack.c
create mode 100644 test/suites/api/test_unpack.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 635176e..659dcb4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -11,7 +11,8 @@ libjansson_la_SOURCES = \
strbuffer.h \
utf.c \
utf.h \
- value.c
+ value.c \
+ variadic.c
libjansson_la_LDFLAGS = \
-export-symbols-regex '^json_' \
-version-info 3:0:3
diff --git a/src/jansson.h b/src/jansson.h
index e463f81..0f1de8d 100644
--- a/src/jansson.h
+++ b/src/jansson.h
@@ -85,6 +85,14 @@ void json_decref(json_t *json)
}


+/* error reporting */
+
+typedef struct json_error_t json_error_t;
+
+const char *json_error_msg(const json_error_t *error);
+int json_error_line(const json_error_t *error);
+
+
/* getters, setters, manipulation */

size_t json_object_size(const json_t *object);
@@ -156,6 +164,8 @@ int json_string_set_nocheck(json_t *string, const char *value);
int json_integer_set(json_t *integer, json_int_t value);
int json_real_set(json_t *real, double value);

+json_t *json_pack(json_error_t **error, const char *fmt, ...);
+int json_unpack(json_t *root, json_error_t **error, const char *fmt, ...);

/* equality */

@@ -168,14 +178,6 @@ json_t *json_copy(json_t *value);
json_t *json_deep_copy(json_t *value);


-/* error reporting */
-
-typedef struct json_error_t json_error_t;
-
-const char *json_error_msg(const json_error_t *error);
-int json_error_line(const json_error_t *error);
-
-
/* loading, printing */

json_t *json_loads(const char *input, size_t flags, json_error_t **error);
diff --git a/src/jansson_private.h b/src/jansson_private.h
index e9102ba..2d2af62 100644
--- a/src/jansson_private.h
+++ b/src/jansson_private.h
@@ -63,4 +63,11 @@ typedef struct {

const object_key_t *jsonp_object_iter_fullkey(void *iter);

+#define JSON_ERROR_MSG_LENGTH 160
+
+struct json_error_t {
+ char msg[JSON_ERROR_MSG_LENGTH];
+ int line;
+};
+
#endif
diff --git a/src/load.c b/src/load.c
index a34fd0c..54bfab8 100644
--- a/src/load.c
+++ b/src/load.c
@@ -60,13 +60,6 @@ typedef struct {

/*** error reporting ***/

-#define JSON_ERROR_MSG_LENGTH 160
-
-struct json_error_t {
- char msg[JSON_ERROR_MSG_LENGTH];
- int line;
-};
-
const char *json_error_msg(const json_error_t *error)
{
return error ? error->msg : NULL;
diff --git a/src/variadic.c b/src/variadic.c
new file mode 100644
index 0000000..2ce110b
--- /dev/null
+++ b/src/variadic.c
@@ -0,0 +1,589 @@
+/*
+ * Copyright (c) 2009, 2010 Petri Lehtinen <pe...@digip.org>
+ * Copyright (c) 2010 Graeme Smecher <graeme....@mail.mcgill.ca>
+ *
+ * Jansson is free software; you can redistribute it and/or modify
+ * it under the terms of the MIT license. See LICENSE for details.
+ */
+
+#include <stdarg.h>
+#include <string.h>
+#include <assert.h>
+
+#include <jansson.h>
+#include "jansson_private.h"
+
+static void error_init(json_error_t **error)
+{
+ if(error)
+ *error = NULL;
+}
+
+static void error_set(json_error_t **error, const int line, const char *msg, ...)
+{
+ va_list ap;
+
+ if(!error || *error)
+ return;
+
+ *error = calloc(1, sizeof(json_error_t));
+ if(!*error)
+ return;
+
+ va_start(ap, msg);
+ vsnprintf((*error)->msg, JSON_ERROR_MSG_LENGTH, msg, ap);
+ va_end(ap);
+
+ va_start(ap, msg);
+ vfprintf(stderr, msg, ap);
+ va_end(ap);
+
+ (*error)->line = line;
+}
+
+json_t *json_pack(json_error_t **error, const char *fmt, ...) {
+ int fmt_length = strlen(fmt);
+ va_list ap;
+
+ /* Keep a stack of containers (lists and objects) */
+ int depth = 0;
+ json_t **stack = NULL;
+
+ /* Keep a list of objects we create in case of error */
+ int free_count = 0;
+ json_t **free_list = NULL;
+
+ json_t *cur = NULL; /* Current container */
+ json_t *root = NULL; /* root object */
+ json_t *obj = NULL;
+
+ char *key = NULL; /* Current key in an object */
+ char *s;
+
+ int line = 1;
+
+ /* Allocation provisioned for worst case */
+ stack = calloc(fmt_length, sizeof(json_t *));
+ free_list = calloc(fmt_length, sizeof(json_t *));
+
+ error_init(error);
+
+ if(!stack || !free_list)
+ goto out;
+
+ va_start(ap, fmt);
+ while(*fmt) {
+ switch(*fmt) {
+ case '\n':
+ line++;
+ break;
+
+ case ' ': /* Whitespace */
+ break;
+
+ case ',': /* Element spacer */
+ if(!root)
+ {
+ error_set(error, line,
+ "Unexpected COMMA precedes root element!");
+ root = NULL;
+ goto out;
+ }
+
+ if(!cur)
+ {
+ error_set(error, line,
+ "Unexpected COMMA outside a list or object!");
+ root = NULL;
+ goto out;
+ }
+
+ if(key)
+ {
+ error_set(error, line, "Expected KEY, got COMMA!");
+ root = NULL;
+ goto out;
+ }
+ break;
+
+ case ':': /* Key/value separator */
+ if(!key)
+ {
+ error_set(error, line, "Got key/value separator without "
+ "a key preceding it!");
+ root = NULL;
+ goto out;
+ }
+
+ if(!json_is_object(cur))
+ {
+ error_set(error, line, "Got a key/value separator "
+ "(':') outside an object!");
+ root = NULL;
+ goto out;
+ }
+
+ break;
+
+ case ']': /* Close array or object */
+ case '}':
+
+ if(key)
+ {
+ error_set(error, line, "OBJECT or ARRAY ended with an "
+ "incomplete key/value pair!");
+ root = NULL;
+ goto out;
+ }
+
+ if(depth <= 0)
+ {
+ error_set(error, line,
+ "Too many close-brackets '%c'", *fmt);
+ root = NULL;
+ goto out;
+ }
+
+ if(*fmt == ']' && !json_is_array(cur))
+ {
+ error_set(error, line,
+ "Stray close-array ']' character");
+ root = NULL;
+ goto out;
+ }
+
+ if(*fmt == '}' && !json_is_object(cur))
+ {
+ error_set(error, line,
+ "Stray close-object '}' character");
+ root = NULL;
+ goto out;
+ }
+
+ cur = stack[--depth];
+ break;
+
+ case '[':
+ obj = json_array();
+ goto obj_common;
+
+ case '{':
+ obj = json_object();
+ goto obj_common;
+
+ case 's': /* string */
+ s = va_arg(ap, char*);
+
+ if(!s)
+ {
+ error_set(error, line,
+ "Refusing to handle a NULL string");
+ root = NULL;
+ goto out;
+ }
+
+ if(json_is_object(cur) && !key)
+ {
+ /* It's a key */
+ key = s;
+ break;
+ }
+
+ obj = json_string(s);
+ goto obj_common;
+
+ case 'n': /* null */
+ obj = json_null();
+ goto obj_common;
+
+ case 'b': /* boolean */
+ obj = va_arg(ap, int) ?
+ json_true() : json_false();
+ goto obj_common;
+
+ case 'i': /* integer */
+ obj = json_integer(va_arg(ap, int));
+ goto obj_common;
+
+ case 'f': /* double-precision float */
+ obj = json_real(va_arg(ap, double));
+ goto obj_common;
+
+ case 'O': /* a json_t object; increments refcount */
+ obj = va_arg(ap, json_t *);
+ json_incref(obj);
+ goto obj_common;
+
+ case 'o': /* a json_t object; doesn't increment refcount */
+ obj = va_arg(ap, json_t *);
+ goto obj_common;
+
+obj_common: free_list[free_count++] = obj;
+
+ /* Root this object to its parent */
+ if(json_is_object(cur)) {
+ if(!key)
+ {
+ error_set(error, line,
+ "Expected key, got identifier '%c'!", *fmt);
+ root = NULL;
+ goto out;
+ }
+
+ json_object_set_new(cur, key, obj);
+ key = NULL;
+ }
+ else if(json_is_array(cur))
+ {
+ json_array_append_new(cur, obj);
+ }
+ else if(!root)
+ {
+ printf("Rooting\n");
+ root = obj;
+ }
+ else
+ {
+ error_set(error, line, "Can't figure out where to attach "
+ "'%c' object!", *fmt);
+ root = NULL;
+ goto out;
+ }
+
+ /* If it was a container ('[' or '{'), descend on the stack */
+ if(json_is_array(obj) || json_is_object(obj))
+ {
+ stack[depth++] = cur;
+ cur = obj;
+ }
+
+ break;
+ }
+ fmt++;
+ }
+ va_end(ap);
+
+ if(depth != 0) {
+ error_set(error, line,
+ "Missing object or array close-brackets in format string");
+ root = NULL;
+ goto out;
+ }
+
+ /* Success: don't free everything we just built! */
+ free_count = 0;
+
+out:
+ while(free_count)
+ json_decref(free_list[--free_count]);
+
+ if(free_list)
+ free(free_list);
+
+ if(stack)
+ free(stack);
+
+ return(root);
+}
+
+int json_unpack(json_t *root, json_error_t **error, const char *fmt, ...) {
+ va_list ap;
+
+ int rv=0; /* Return value */
+ int line = 1; /* Line number */
+
+ /* Keep a stack of containers (lists and objects) */
+ int depth = 0;
+ json_t **stack;
+
+ int array_index = 0;
+ char *key = NULL; /* Current key in an object */
+
+ json_t *cur = NULL; /* Current container */
+ json_t *obj = NULL;
+
+ int fmt_length = strlen(fmt);
+
+ error_init(error);
+
+ /* Allocation provisioned for worst case */
+ stack = calloc(fmt_length, sizeof(json_t *));
+ if(!stack)
+ {
+ error_set(error, line, "Out of memory!");
+ rv = -1;
+ goto out;
+ }
+
+ /* Even if we're successful, we need to know if the number of
+ * arguments provided matches the number of JSON objects.
+ * We can do this by counting the elements in every array or
+ * object we open up, and decrementing the count as we visit
+ * their children. */
+ int unvisited = 0;
+
+ va_start(ap, fmt);
+ while(*fmt)
+ {
+ switch(*fmt)
+ {
+ case ' ': /* Whitespace */
+ break;
+
+ case '\n': /* Line break */
+ line++;
+ break;
+
+ case ',': /* Element spacer */
+
+ if(!cur)
+ {
+ error_set(error, line,
+ "Unexpected COMMA outside a list or object!");
+ rv = -1;
+ goto out;
+ }
+
+ if(key)
+ {
+ error_set(error, line, "Expected KEY, got COMMA!");
+ rv = -1;
+ goto out;
+ }
+ break;
+
+ case ':': /* Key/value separator */
+ if(!json_is_object(cur) || !key)
+ {
+ error_set(error, line, "Unexpected ':'");
+ rv = -1;
+ goto out;
+ }
+ break;
+
+ case '[':
+ case '{':
+ /* Fetch object */
+ if(!cur)
+ {
+ obj = root;
+ }
+ else if(json_is_object(cur))
+ {
+ if(!key)
+ {
+ error_set(error, line, "Objects can't be keys");
+ rv = -1;
+ goto out;
+ }
+ obj = json_object_get(cur, key);
+ unvisited--;
+ key = NULL;
+ }
+ else if(json_is_array(cur))
+ {
+ obj = json_array_get(cur, array_index);
+ unvisited--;
+ array_index++;
+ }
+ else
+ {
+ assert(0);
+ }
+
+ /* Make sure we got what we expected */
+ if(*fmt=='{' && !json_is_object(obj))
+ {
+ rv = -2;
+ goto out;
+ }
+
+ if(*fmt=='[' && !json_is_array(obj))
+ {
+ rv = -2;
+ goto out;
+ }
+
+ unvisited += json_is_object(obj) ?
+ json_object_size(obj) :
+ json_array_size(obj);
+
+ /* Descend */
+ stack[depth++] = cur;
+ cur = obj;
+
+ key = NULL;
+
+ break;
+
+
+ case ']':
+ case '}':
+
+ if(json_is_array(cur) && *fmt!=']')
+ {
+ error_set(error, line, "Missing ']'");
+ rv = -1;
+ goto out;
+ }
+
+ if(json_is_object(cur) && *fmt!='}')
+ {
+ error_set(error, line, "Missing '}'");
+ rv = -1;
+ goto out;
+ }
+
+ if(key)
+ {
+ error_set(error, line, "Unexpected '%c'", *fmt);
+ rv = -1;
+ goto out;
+ }
+
+ if(depth <= 0)
+ {
+ error_set(error, line, "Unexpected '%c'", *fmt);
+ rv = -1;
+ goto out;
+ }
+
+ cur = stack[--depth];
+
+ break;
+
+ case 's':
+ if(!key && json_is_object(cur))
+ {
+ /* constant string for key */
+ key = va_arg(ap, char*);
+ break;
+ }
+ /* fall through */
+
+ case 'i': /* integer */
+ case 'f': /* double-precision float */
+ case 'O': /* a json_t object; increments refcount */
+ case 'o': /* a json_t object; borrowed reference */
+ case 'b': /* boolean */
+ case 'n': /* null */
+
+ /* Fetch object */
+ if(!cur)
+ {
+ obj = root;
+ }
+ else if(json_is_object(cur))
+ {
+ if(!key)
+ {
+ error_set(error, line,
+ "Only strings may be used as keys!");
+ rv = -1;
+ goto out;
+ }
+
+ obj = json_object_get(cur, key);
+ unvisited--;
+ key = NULL;
+ }
+ else if(json_is_array(cur))
+ {
+ obj = json_array_get(cur, array_index);
+ unvisited--;
+ array_index++;
+ }
+ else
+ {
+ error_set(error, line,
+ "Unsure how to retrieve JSON object '%c'",
+ *fmt);
+ rv = -1;
+ goto out;
+ }
+
+ switch(*fmt)
+ {
+ case 's':
+ if(!json_is_string(obj))
+ {
+ error_set(error, line,
+ "Type mismatch! Object wasn't a string.");
+ rv = -2;
+ goto out;
+ }
+ *va_arg(ap, const char**) = json_string_value(obj);
+ break;
+
+ case 'i':
+ if(!json_is_integer(obj))
+ {
+ error_set(error, line,
+ "Type mismatch! Object wasn't an integer.");
+ rv = -2;
+ goto out;
+ }
+ *va_arg(ap, int*) = json_integer_value(obj);
+ break;
+
+ case 'b':
+ if(!json_is_boolean(obj))
+ {
+ error_set(error, line,
+ "Type mismatch! Object wasn't a boolean.");
+ rv = -2;
+ goto out;
+ }
+ *va_arg(ap, int*) = json_is_true(obj);
+ break;
+
+ case 'f':
+ if(!json_is_number(obj))
+ {
+ error_set(error, line,
+ "Type mismatch! Object wasn't a real.");
+ rv = -2;
+ goto out;
+ }
+ *va_arg(ap, double*) = json_number_value(obj);
+ break;
+
+ case 'O':
+ json_incref(obj);
+ /* Fall through */
+
+ case 'o':
+ *va_arg(ap, json_t**) = obj;
+ break;
+
+ case 'n':
+ /* Don't actually assign anything; we're just happy
+ * the null turned up as promised in the format
+ * string. */
+ break;
+
+ default:
+ error_set(error, line,
+ "Unknown format character '%c'", *fmt);
+ rv = -1;
+ goto out;
+ }
+ }
+ fmt++;
+ }
+
+ /* Return 0 if everything was matched; otherwise the number of JSON
+ * objects we didn't get to. */
+ rv = unvisited;
+
+out:
+ va_end(ap);
+
+ if(stack)
+ free(stack);
+
+ return(rv);
+}
+
+/* vim: ts=4:expandtab:sw=4
+ */
diff --git a/test/.gitignore b/test/.gitignore
index dca3c1e..cb88169 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -9,3 +9,5 @@ suites/api/test_number
suites/api/test_object
suites/api/test_simple
suites/api/test_cpp
+suites/api/test_pack
+suites/api/test_unpack
diff --git a/test/suites/api/Makefile.am b/test/suites/api/Makefile.am
index 7125792..ce8be4a 100644
--- a/test/suites/api/Makefile.am
+++ b/test/suites/api/Makefile.am
@@ -8,7 +8,9 @@ check_PROGRAMS = \
test_load \
test_simple \
test_number \
- test_object
+ test_object \
+ test_pack \
+ test_unpack

test_array_SOURCES = test_array.c util.h
test_copy_SOURCES = test_copy.c util.h
@@ -17,6 +19,7 @@ test_load_SOURCES = test_load.c util.h
test_simple_SOURCES = test_simple.c util.h
test_number_SOURCES = test_number.c util.h
test_object_SOURCES = test_object.c util.h
+test_variadic_SOURCES = test_variadic.c util.h

AM_CPPFLAGS = -I$(top_srcdir)/src
AM_CFLAGS = -Wall -Werror
diff --git a/test/suites/api/check-exports b/test/suites/api/check-exports
index b1ef400..af22c28 100755
--- a/test/suites/api/check-exports
+++ b/test/suites/api/check-exports
@@ -55,6 +55,8 @@ json_load_file
json_equal
json_copy
json_deep_copy
+json_pack
+json_unpack
EOF

# The list of functions are not exported in the library because they
diff --git a/test/suites/api/test_pack.c b/test/suites/api/test_pack.c
new file mode 100644
index 0000000..f1ca388
--- /dev/null
+++ b/test/suites/api/test_pack.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2009, 2010 Petri Lehtinen <pe...@digip.org>
+ * Copyright (c) 2010 Graeme Smecher <graeme....@mail.mcgill.ca>
+ *
+ * Jansson is free software; you can redistribute it and/or modify
+ * it under the terms of the MIT license. See LICENSE for details.
+ */
+
+#include <string.h>
+#include <jansson.h>
+#include <stdio.h>
+#include "util.h"
+
+int main()
+{
+ json_t *value;
+ int i;
+
+ /*
+ * Simple, valid json_pack cases
+ */
+
+ /* true */
+ value = json_pack(NULL, "b", 1);
+ if(!json_is_true(value))
+ fail("json_pack boolean failed");
+ if(value->refcount != (ssize_t)-1)
+ fail("json_pack boolean refcount failed");
+ json_decref(value);
+
+ /* false */
+ value = json_pack(NULL, "b", 0);
+ if(!json_is_false(value))
+ fail("json_pack boolean failed");
+ if(value->refcount != (ssize_t)-1)
+ fail("json_pack boolean refcount failed");
+ json_decref(value);
+
+ /* null */
+ value = json_pack(NULL, "n");
+ if(!json_is_null(value))
+ fail("json_pack null failed");
+ if(value->refcount != (ssize_t)-1)
+ fail("json_pack null refcount failed");
+ json_decref(value);
+
+ /* integer */
+ value = json_pack(NULL, "i", 1);
+ if(!json_is_integer(value) || json_integer_value(value) != 1)
+ fail("json_pack integer failed");
+ if(value->refcount != (ssize_t)1)
+ fail("json_pack integer refcount failed");
+ json_decref(value);
+
+
+ /* real */
+ value = json_pack(NULL, "f", 1.0);
+ if(!json_is_real(value) || json_real_value(value) != 1.0)
+ fail("json_pack real failed");
+ if(value->refcount != (ssize_t)1)
+ fail("json_pack real refcount failed");
+ json_decref(value);
+
+ /* string */
+ value = json_pack(NULL, "s", "test");
+ if(!json_is_string(value) || strcmp("test", json_string_value(value)))
+ fail("json_pack string failed");
+ if(value->refcount != (ssize_t)1)
+ fail("json_pack string refcount failed");
+ json_decref(value);
+
+ /* empty object */
+ value = json_pack(NULL, "{}", 1.0);
+ if(!json_is_object(value) || json_object_size(value) != 0)
+ fail("json_pack empty object failed");
+ if(value->refcount != (ssize_t)1)
+ fail("json_pack empty object refcount failed");
+ json_decref(value);
+
+ /* empty list */
+ value = json_pack(NULL, "[]", 1.0);
+ if(!json_is_array(value) || json_array_size(value) != 0)
+ fail("json_pack empty list failed");
+ if(value->refcount != (ssize_t)1)
+ fail("json_pack empty list failed");
+ json_decref(value);
+
+ /* non-incref'd object */
+ value = json_pack(NULL, "o", json_integer(1));
+ if(!json_is_integer(value) || json_integer_value(value) != 1)
+ fail("json_pack object failed");
+ if(value->refcount != (ssize_t)1)
+ fail("json_pack integer refcount failed");
+ json_decref(value);
+
+ /* incref'd object */
+ value = json_pack(NULL, "O", json_integer(1));
+ if(!json_is_integer(value) || json_integer_value(value) != 1)
+ fail("json_pack object failed");
+ if(value->refcount != (ssize_t)2)
+ fail("json_pack integer refcount failed");
+ json_decref(value);
+ json_decref(value);
+
+ /* simple object */
+ value = json_pack(NULL, "{s:[]}", "foo");
+ if(!json_is_object(value) || json_object_size(value) != 1)
+ fail("json_pack object failed");
+ if(!json_is_array(json_object_get(value, "foo")))
+ fail("json_pack object failed");
+ if(json_object_get(value, "foo")->refcount != (ssize_t)1)
+ fail("json_pack object refcount failed");
+ json_decref(value);
+
+ /* simple array */
+ value = json_pack(NULL, "[i,i,i]", 0, 1, 2);
+ if(!json_is_array(value) || json_array_size(value) != 3)
+ fail("json_pack object failed");
+ for(i=0; i<3; i++)
+ {
+ if(!json_is_integer(json_array_get(value, i)) ||
+ json_integer_value(json_array_get(value, i)) != i)
+
+ fail("json_pack integer array failed");
+ }
+ json_decref(value);
+
+ /*
+ * Invalid cases
+ */
+
+ /* mismatched open/close array/object */
+ if(json_pack(NULL, "[}"))
+ fail("json_pack failed to catch mismatched '}'");
+
+ if(json_pack(NULL, "{]"))
+ fail("json_pack failed to catch mismatched ']'");
+
+ /* missing close array */
+ if(json_pack(NULL, "["))
+ fail("json_pack failed to catch missing ']'");
+
+ /* missing close object */
+ if(json_pack(NULL, "{"))
+ fail("json_pack failed to catch missing '}'");
+
+ /* NULL string */
+ if(json_pack(NULL, "s", NULL))
+ fail("json_pack failed to catch null string");
+
+ return(0);
+}
+
+/* vim: ts=4:expandtab:sw=4
+ */
diff --git a/test/suites/api/test_unpack.c b/test/suites/api/test_unpack.c
new file mode 100644
index 0000000..ea18ff8
--- /dev/null
+++ b/test/suites/api/test_unpack.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2009, 2010 Petri Lehtinen <pe...@digip.org>
+ * Copyright (c) 2010 Graeme Smecher <graeme....@mail.mcgill.ca>
+ *
+ * Jansson is free software; you can redistribute it and/or modify
+ * it under the terms of the MIT license. See LICENSE for details.
+ */
+
+#include <string.h>
+#include <jansson.h>
+#include <stdio.h>
+#include "util.h"
+
+int main()
+{
+ json_t *j, *j2;
+ int i1, i2, i3;
+ int rv;
+ //void* v;
+ double f;
+ char *s;
+
+ /*
+ * Simple, valid json_pack cases
+ */
+
+ /* true */
+ rv = json_unpack(json_true(), NULL, "b", &i1);
+ if(rv || !i1)
+ fail("json_unpack boolean failed");
+
+ /* false */
+ rv = json_unpack(json_false(), NULL, "b", &i1);
+ if(rv || i1)
+ fail("json_unpack boolean failed");
+
+ /* null */
+ rv = json_unpack(json_null(), NULL, "n");
+ if(rv)
+ fail("json_unpack null failed");
+
+ /* integer */
+ j = json_integer(1);
+ rv = json_unpack(j, NULL, "i", &i1);
+ if(rv || i1 != 1)
+ fail("json_unpack integer failed");
+ json_decref(j);
+
+ /* real */
+ j = json_real(1.0);
+ rv = json_unpack(j, NULL, "f", &f);
+ if(rv || f != 1.0)
+ fail("json_unpack real failed");
+ json_decref(j);
+
+ /* string */
+ j = json_string("foo");
+ rv = json_unpack(j, NULL, "s", &s);
+ if(rv || strcmp(s, "foo"))
+ fail("json_unpack string failed");
+ json_decref(j);
+
+ /* empty object */
+ j = json_object();
+ rv = json_unpack(j, NULL, "{}");
+ if(rv)
+ fail("json_unpack empty object failed");
+ json_decref(j);
+
+ /* empty list */
+ j = json_array();
+ rv = json_unpack(j, NULL, "[]");
+ if(rv)
+ fail("json_unpack empty list failed");
+ json_decref(j);
+
+ /* non-incref'd object */
+ j = json_object();
+ rv = json_unpack(j, NULL, "o", &j2);
+ if(j2 != j || j->refcount != (ssize_t)1)
+ fail("json_unpack object failed");
+ json_decref(j);
+
+ /* incref'd object */
+ j = json_object();
+ rv = json_unpack(j, NULL, "O", &j2);
+ if(j2 != j || j->refcount != (ssize_t)2)
+ fail("json_unpack object failed");
+ json_decref(j);
+ json_decref(j);
+
+ /* simple object */
+ j = json_pack(NULL, "{s:i}", "foo", 1);
+ rv = json_unpack(j, NULL, "{s:i}", "foo", &i1);
+ if(rv || i1!=1)
+ fail("json_unpack simple object failed");
+ json_decref(j);
+
+ /* simple array */
+ j = json_pack(NULL, "[iii]", 1, 2, 3);
+ rv = json_unpack(j, NULL, "[i,i,i]", &i1, &i2, &i3);
+ if(rv || i1 != 1 || i2 != 2 || i3 != 3)
+ fail("json_unpack simple array failed");
+ json_decref(j);
+
+ return 0;
+}
+
+/* vim: ts=4:expandtab:sw=4
+ */
--
1.7.1

[Graeme Smecher]

Hi Petri,

I just sent a patch that integrates json_pack and json_pack into the
tree. It's a halfway measure: it addresses some of the changes you had
in mind, and leaves others for later. The code fits in your tree now,
and has some rudimentary test cases. It's not bombproof yet, but
hopefully it'll improve quickly.

Comments in-line.

> if(json_unpack(value, "[i, f]",&intval,&doubleval, NULL)) {

> /* an error occured, no error information returned */
> }
>
> The return value can be e.g. -1 for matching errors and -2 for format
> string errors, but in both cases we could return an error message. The
> line could be the line *in the format string*, on which the
> mismatching format character was found.
>

It would perhaps be more useful to encode the character offset in some
way, since I don't anticipate many multiline format strings. I'd imagine
this would be useful in the "loads" method as well. At any rate, the
line number is now emitted with error structures in the code.

The changes also maintain the current behaviour (-1 or -2 indicates an
error; 0 indicates a complete match, and >0 indicates extra inputs that
weren't matched.)

> And more food for thought: To make json_unpack() much more powerful
> for data extraction, it should be possible to match only a part of an
> array or, more importantly, an object. For this reason, a "..." format
> specifier could be supported:
>

> json_unpack(value, "{s: i, s: f, ...}", "foo",&i, "bar",&d, NULL);

>
> This would allow the matching to succeed even if there are more keys
> than just "foo" and "bar" in the object. For arrays this, would work
> by matching only the first few values and not caring about the rest.
>

This is an alternative to the >0 return codes. I know the ellipsis "..."
is conventional, but it would be nice to fit this into a
single-character sequence, but nothing suggests itself to me offhand.
Otherwise, it'd be the only multicharacter token.

>
>> - There shouldn't be a separate format character for object keys. With
>> a bit of logic, 's' can be used, like this:
>>
>> json_pack("{s:f}", "pi", 3.141592);
>>
>> ==> {"pi": 3.141592}
>>
>>
>> I had originally coded things this way. It's mostly a matter of taste, but
>> using "s" for both constant strings (char *) and string arguments (char **)
>> during json_unpack() calls seemed a little overloaded. Having changed the code
>> for keys "k", though, I keep writing keys as "s" anyways and realizing my
>> error. I don't feel strongly about it.
>>
> I think it should be "s" for keys. If starts to feel confusing at some
> point, it's easy to allow "k" for keys as well... Or drop the
> requirement to specify a format character for object keys at all, see
> below.
>

Done -- I think "s" is the right approach (rather than "k"). You hint at
an alternative to keys at all -- can you elaborate?

>
> I'm looking forward to see this in the Jansson tree, but I'm not sure
> if this email smoothened the API or added more rough edges :)
>

Well, keep the comments coming! I need to do more testing of error
cases, and I'm obnoxiously sticking with a nonrecursive implementation
until you nudge me a little harder.

cheers,
Graeme

[Petri Lehtinen]

Graeme Smecher wrote:
> Hi Petri,
>
> I just sent a patch that integrates json_pack and json_pack into the
> tree. It's a halfway measure: it addresses some of the changes you
> had in mind, and leaves others for later. The code fits in your tree
> now, and has some rudimentary test cases. It's not bombproof yet,
> but hopefully it'll improve quickly.

Fixed errors test/suites/api/Makefile.am, removed some extra
whitespace from variadic.c and pushed to master.

If you have used Jansson 1.3, you may have noticed that I refactored
the error reporting in the master branch a few days ago and you had to
move the json_error_t struct around. After looking at the new error
reporting code for a few days, I don't like it anymore so I might
change it back to how it was, perhaps adding some fields to the
json_error_t struct. But you don't have to worry about that.

> Comments in-line.

[snip]

> It would perhaps be more useful to encode the character offset in
> some way, since I don't anticipate many multiline format strings.
> I'd imagine this would be useful in the "loads" method as well. At
> any rate, the line number is now emitted with error structures in
> the code.

Yes, and this would need a new field to json_error_t. I'll look into
it.

> The changes also maintain the current behaviour (-1 or -2 indicates
> an error; 0 indicates a complete match, and >0 indicates extra
> inputs that weren't matched.)
>
> >And more food for thought: To make json_unpack() much more powerful
> >for data extraction, it should be possible to match only a part of an
> >array or, more importantly, an object. For this reason, a "..." format
> >specifier could be supported:
> >
> > json_unpack(value, "{s: i, s: f, ...}", "foo",&i, "bar",&d, NULL);
> >
> >This would allow the matching to succeed even if there are more keys
> >than just "foo" and "bar" in the object. For arrays this, would work
> >by matching only the first few values and not caring about the rest.
>
> This is an alternative to the >0 return codes. I know the ellipsis
> "..." is conventional, but it would be nice to fit this into a
> single-character sequence, but nothing suggests itself to me
> offhand. Otherwise, it'd be the only multicharacter token.

You're right, but something like the ellipsis is needed. With only the
return value, you cannot know which of the nested objects or arrays
contained extra elements.

"..." is a bit hard to parse and it's not in line with other format
characters, so how about "-":

json_unpack(value, "[if-]", &error, &intval, &doubleval);

And I'm afraid that supporting this in nested objects and arrays
requires the recursive parser... :)

> Done -- I think "s" is the right approach (rather than "k"). You
> hint at an alternative to keys at all -- can you elaborate?

I had a rather long description of it in the earlier mail. Did you not
notice it? My point was that because in JSON, object keys are always
strings, we could do this:

json_pack("{ii}", "foo", 42, "bar", 43);

i.e. omit the "s" or "k" for object keys because they're always
strings anyway.

But after rethinking, this is probably too confusing as there are more
varargs than format characters. So making all delimiters optional and
having a separate format character for keys and values is good, just
like it's implemented now. You don't even have to check them as
strictly as you do now, they could simply be skipped as Python does
(if I read the documentation correctly).

> >I'm looking forward to see this in the Jansson tree, but I'm not sure
> >if this email smoothened the API or added more rough edges :)
>
> Well, keep the comments coming! I need to do more testing of error
> cases, and I'm obnoxiously sticking with a nonrecursive
> implementation until you nudge me a little harder.

I'd like the error messages be a bit more verbose. It would be very
helpful for the user to see which element had a mismatch, i.e. what
was the array index or object key corresponding to the mismatched
value.

Petri

[Graeme Smecher]

Hi Petri,

On 25/10/10 11:29 PM, Petri Lehtinen wrote:
> [snip]

>> This is an alternative to the>0 return codes. I know the ellipsis
>> "..." is conventional, but it would be nice to fit this into a
>> single-character sequence, but nothing suggests itself to me
>> offhand. Otherwise, it'd be the only multicharacter token.
>>
> You're right, but something like the ellipsis is needed. With only the
> return value, you cannot know which of the nested objects or arrays
> contained extra elements.
>
> "..." is a bit hard to parse and it's not in line with other format
> characters, so how about "-":
>
> json_unpack(value, "[if-]",&error,&intval,&doubleval);
>
> And I'm afraid that supporting this in nested objects and arrays
> requires the recursive parser... :)
>

OK, OK, you win. :)

>> Done -- I think "s" is the right approach (rather than "k"). You
>> hint at an alternative to keys at all -- can you elaborate?
>>
> I had a rather long description of it in the earlier mail. Did you not
> notice it? My point was that because in JSON, object keys are always
> strings, we could do this:
>
> json_pack("{ii}", "foo", 42, "bar", 43);
>
> i.e. omit the "s" or "k" for object keys because they're always
> strings anyway.
>
> But after rethinking, this is probably too confusing as there are more
> varargs than format characters. So making all delimiters optional and
> having a separate format character for keys and values is good, just
> like it's implemented now. You don't even have to check them as
> strictly as you do now, they could simply be skipped as Python does
> (if I read the documentation correctly).
>

Sorry, your earlier description tucked nicely into my blind spot. I
agree with the above, and will relax the checks for separators between
keys and values.

>>> I'm looking forward to see this in the Jansson tree, but I'm not sure
>>> if this email smoothened the API or added more rough edges :)
>>>
>> Well, keep the comments coming! I need to do more testing of error
>> cases, and I'm obnoxiously sticking with a nonrecursive
>> implementation until you nudge me a little harder.
>>
> I'd like the error messages be a bit more verbose. It would be very
> helpful for the user to see which element had a mismatch, i.e. what
> was the array index or object key corresponding to the mismatched
> value.
>

I agree. I'd also like to make it absolutely clear whether the error
originated with JSON or the formatting string (it is probably ambiguous
at the moment.)

I will try to follow up in a couple of days. Thanks for pushing this
forward so quickly!

cheers,
Graeme

[Petri Lehtinen]

Graeme Smecher wrote:
> I will try to follow up in a couple of days. Thanks for pushing this
> forward so quickly!

I refactored the error reporting in master. Hope you don't have a hard
time rebasing on it...

Petri

[gsme...@gmail.com]

From: Graeme Smecher <graeme....@mail.mcgill.ca>

This change will facilitate the use of a recursive-descent parser for both
json_pack and json_unpack. We also add some error-checking, implement the
new json_error_t "column" field, and remove a debugging printf that snuck into
an earlier commit.
---
src/variadic.c | 173 ++++++++++++++++++++++++++++++++++---------------------
1 files changed, 107 insertions(+), 66 deletions(-)

diff --git a/src/variadic.c b/src/variadic.c
index 89ff1da..58974ee 100644
--- a/src/variadic.c
+++ b/src/variadic.c
@@ -13,9 +13,8 @@
#include <jansson.h>
#include "jansson_private.h"

-json_t *json_pack(json_error_t *error, const char *fmt, ...) {
- int fmt_length = strlen(fmt);
- va_list ap;
+static json_t *json_vnpack(json_error_t *error, ssize_t size, const char *fmt, va_list ap)

+{

/* Keep a stack of containers (lists and objects) */

int depth = 0;
@@ -29,23 +28,22 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

json_t *root = NULL; /* root object */

json_t *obj = NULL;

+ const char *tok = fmt;

+
char *key = NULL; /* Current key in an object */

char *s;

int line = 1;

/* Allocation provisioned for worst case */

- stack = calloc(fmt_length, sizeof(json_t *));
- free_list = calloc(fmt_length, sizeof(json_t *));
-
- jsonp_error_init(error, "");
+ stack = calloc(size, sizeof(json_t *));
+ free_list = calloc(size, sizeof(json_t *));

if(!stack || !free_list)
goto out;

- va_start(ap, fmt);
- while(*fmt) {
- switch(*fmt) {
+ while(tok-fmt < size) {
+ switch(*tok) {
case '\n':
line++;
break;
@@ -56,7 +54,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

case ',': /* Element spacer */

if(!root)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Unexpected COMMA precedes root element!");

root = NULL;
goto out;
@@ -64,7 +62,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

if(!cur)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Unexpected COMMA outside a list or object!");

root = NULL;
goto out;
@@ -72,7 +70,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

if(key)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Expected KEY, got COMMA!");

root = NULL;
goto out;
@@ -82,7 +80,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

case ':': /* Key/value separator */

if(!key)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,
"Got key/value separator without "

"a key preceding it!");

root = NULL;
@@ -91,7 +89,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

if(!json_is_object(cur))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,
"Got a key/value separator "

"(':') outside an object!");

root = NULL;
@@ -105,7 +103,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

if(key)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"OBJECT or ARRAY ended with an "

"incomplete key/value pair!");
root = NULL;
@@ -114,23 +112,23 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

if(depth <= 0)
{
- jsonp_error_set(error, line, -1,
- "Too many close-brackets '%c'", *fmt);
+ jsonp_error_set(error, line, tok-fmt,
+ "Too many close-brackets '%c'", *tok);
root = NULL;
goto out;
}

- if(*fmt == ']' && !json_is_array(cur))
+ if(*tok == ']' && !json_is_array(cur))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,
"Stray close-array ']' character");
root = NULL;
goto out;
}

- if(*fmt == '}' && !json_is_object(cur))
+ if(*tok == '}' && !json_is_object(cur))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,
"Stray close-object '}' character");
root = NULL;
goto out;
@@ -152,7 +150,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...) {

if(!s)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Refusing to handle a NULL string");

root = NULL;
goto out;
@@ -200,8 +198,8 @@ obj_common: free_list[free_count++] = obj;
if(json_is_object(cur)) {
if(!key)
{
- jsonp_error_set(error, line, -1,
- "Expected key, got identifier '%c'!", *fmt);
+ jsonp_error_set(error, line, tok-fmt,
+ "Expected key, got identifier '%c'!", *tok);
root = NULL;
goto out;
}
@@ -215,14 +213,13 @@ obj_common: free_list[free_count++] = obj;
}
else if(!root)
{
- printf("Rooting\n");
root = obj;
}
else
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Can't figure out where to attach "

- "'%c' object!", *fmt);
+ "'%c' object!", *tok);
root = NULL;
goto out;
}
@@ -236,12 +233,11 @@ obj_common: free_list[free_count++] = obj;

break;
}
- fmt++;
+ tok++;
}
- va_end(ap);

if(depth != 0) {
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Missing object or array close-brackets in format string");

root = NULL;
goto out;
@@ -263,15 +259,15 @@ out:
return(root);
}

-int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
- va_list ap;
+static int json_vnunpack(json_t *root, json_error_t *error, ssize_t size, const char *fmt, va_list ap)

+{

int rv=0; /* Return value */

int line = 1; /* Line number */

/* Keep a stack of containers (lists and objects) */

int depth = 0;
- json_t **stack;

+ json_t **stack = NULL;

int array_index = 0;

char *key = NULL; /* Current key in an object */

@@ -279,15 +275,13 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {

json_t *cur = NULL; /* Current container */

json_t *obj = NULL;

- int fmt_length = strlen(fmt);
-
- jsonp_error_init(error, "");
+ const char *tok = fmt;

/* Allocation provisioned for worst case */

- stack = calloc(fmt_length, sizeof(json_t *));
+ stack = calloc(size, sizeof(json_t *));
if(!stack)
{
- jsonp_error_set(error, line, -1, "Out of memory!");
+ jsonp_error_set(error, line, 0, "Out of memory!");

rv = -1;
goto out;
}

@@ -299,10 +293,9 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {

* their children. */
int unvisited = 0;

- va_start(ap, fmt);
- while(*fmt)
+ while(tok-fmt < size)
{
- switch(*fmt)
+ switch(*tok)
{
case ' ': /* Whitespace */
break;
@@ -315,7 +308,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {

if(!cur)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Unexpected COMMA outside a list or object!");

rv = -1;
goto out;

@@ -323,7 +316,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {

if(key)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Expected KEY, got COMMA!");

rv = -1;
goto out;

@@ -333,7 +326,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {

case ':': /* Key/value separator */

if(!json_is_object(cur) || !key)
{
- jsonp_error_set(error, line, -1, "Unexpected ':'");
+ jsonp_error_set(error, line, tok-fmt, "Unexpected ':'");

rv = -1;
goto out;
}

@@ -350,7 +343,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
{
if(!key)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Objects can't be keys");

rv = -1;
goto out;

@@ -371,14 +364,18 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {

}

/* Make sure we got what we expected */

- if(*fmt=='{' && !json_is_object(obj))
+ if(*tok=='{' && !json_is_object(obj))
{
+ jsonp_error_set(error, line, tok-fmt,
+ "Token '{' didn't correspond to an object");

rv = -2;
goto out;
}

- if(*fmt=='[' && !json_is_array(obj))
+ if(*tok=='[' && !json_is_array(obj))
{
+ jsonp_error_set(error, line, tok-fmt,
+ "Token '[' didn't correspond to an object");

rv = -2;
goto out;
}

@@ -399,30 +396,30 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
case ']':
case '}':

- if(json_is_array(cur) && *fmt!=']')
+ if(json_is_array(cur) && *tok!=']')
{
- jsonp_error_set(error, line, -1, "Missing ']'");
+ jsonp_error_set(error, line, tok-fmt, "Missing ']'");

rv = -1;
goto out;
}

- if(json_is_object(cur) && *fmt!='}')
+ if(json_is_object(cur) && *tok!='}')
{
- jsonp_error_set(error, line, -1, "Missing '}'");
+ jsonp_error_set(error, line, tok-fmt, "Missing '}'");

rv = -1;
goto out;
}

if(key)
{
- jsonp_error_set(error, line, -1, "Unexpected '%c'", *fmt);
+ jsonp_error_set(error, line, tok-fmt, "Unexpected '%c'", *tok);

rv = -1;
goto out;
}

if(depth <= 0)
{
- jsonp_error_set(error, line, -1, "Unexpected '%c'", *fmt);
+ jsonp_error_set(error, line, tok-fmt, "Unexpected '%c'", *tok);

rv = -1;
goto out;
}

@@ -456,7 +453,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
{
if(!key)
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Only strings may be used as keys!");

rv = -1;
goto out;

@@ -474,19 +471,19 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
}
else
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Unsure how to retrieve JSON object '%c'",

- *fmt);
+ *tok);

rv = -1;
goto out;
}

- switch(*fmt)
+ switch(*tok)
{
case 's':
if(!json_is_string(obj))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Type mismatch! Object wasn't a string.");

rv = -2;
goto out;

@@ -497,7 +494,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
case 'i':
if(!json_is_integer(obj))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Type mismatch! Object wasn't an integer.");

rv = -2;
goto out;

@@ -508,7 +505,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
case 'b':
if(!json_is_boolean(obj))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Type mismatch! Object wasn't a boolean.");

rv = -2;
goto out;

@@ -519,7 +516,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
case 'f':
if(!json_is_number(obj))
{
- jsonp_error_set(error, line, -1,
+ jsonp_error_set(error, line, tok-fmt,

"Type mismatch! Object wasn't a real.");

rv = -2;
goto out;

@@ -542,13 +539,20 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
break;

default:
- jsonp_error_set(error, line, -1,
- "Unknown format character '%c'", *fmt);
+ jsonp_error_set(error, line, tok-fmt,
+ "Unknown format character '%c'", *tok);

rv = -1;
goto out;
}
}

- fmt++;
+ tok++;
+ }

+
+ if(depth != 0) {

+ jsonp_error_set(error, line, tok-fmt,

+ "Missing object or array close-brackets in format string");

+ rv = -2;
+ goto out;
}

/* Return 0 if everything was matched; otherwise the number of JSON

@@ -556,7 +560,6 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...) {
rv = unvisited;

out:
- va_end(ap);

if(stack)
free(stack);
@@ -564,5 +567,43 @@ out:
return(rv);
}

+json_t *json_pack(json_error_t *error, const char *fmt, ...)
+{
+ va_list ap;
+ json_t *obj;
+
+ jsonp_error_init(error, "");
+
+ if(!fmt || !*fmt) {
+ jsonp_error_set(error, 1, 0, "Null or empty format string!");
+ return(NULL);
+ }
+
+ va_start(ap, fmt);
+ obj = json_vnpack(error, strlen(fmt), fmt, ap);
+ va_end(ap);
+
+ return(obj);
+}
+
+int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...)
+{
+ va_list ap;
+ int rv;
+
+ jsonp_error_init(error, "");
+
+ if(!fmt || !*fmt) {
+ jsonp_error_set(error, 1, 0, "Null or empty format string!");
+ return(-2);;
+ }
+
+ va_start(ap, fmt);
+ rv = json_vnunpack(root, error, strlen(fmt), fmt, ap);
+ va_end(ap);
+
+ return(rv);
+}
+
/* vim: ts=4:expandtab:sw=4
*/
--
1.7.2.3

[gsme...@gmail.com]

From: Graeme Smecher <graeme....@mail.mcgill.ca>

---
test/suites/api/test_unpack.c | 38 ++++++++++++++++++++++++++++++++++++++
1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/test/suites/api/test_unpack.c b/test/suites/api/test_unpack.c
index ea18ff8..933ed2f 100644
--- a/test/suites/api/test_unpack.c
+++ b/test/suites/api/test_unpack.c
@@ -103,6 +103,44 @@ int main()

fail("json_unpack simple array failed");

json_decref(j);

+ /*
+ * Invalid cases
+ */
+
+ /* mismatched open/close array/object */

+ j = json_pack(NULL, "[]");
+ rv = json_unpack(j, NULL, "[}");
+ if(!rv)
+ fail("json_unpack failed to catch mismatched ']'");
+ json_decref(j);
+
+ j = json_pack(NULL, "{}");
+ rv = json_unpack(j, NULL, "{]");
+ if(!rv)
+ fail("json_unpack failed to catch mismatched '}'");
+ json_decref(j);

+
+ /* missing close array */

+ j = json_pack(NULL, "[]");
+ rv = json_unpack(j, NULL, "[");
+ if(rv >= 0)
+ fail("json_unpack failed to catch missing ']'");
+ json_decref(j);

+
+ /* missing close object */

+ j = json_pack(NULL, "{}");
+ rv = json_unpack(j, NULL, "{");
+ if(rv >= 0)
+ fail("json_unpack failed to catch missing '}'");
+ json_decref(j);

+
+ /* NULL string */

+ j = json_pack(NULL, "[]");
+ rv =json_unpack(j, NULL, NULL);
+ if(rv >= 0)
+ fail("json_unpack failed to catch null string");
+ json_decref(j);
+
return 0;
}

--
1.7.2.3

[gsme...@gmail.com]

From: Graeme Smecher <graeme....@mail.mcgill.ca>

---
src/variadic.c | 773 ++++++++++++++++++++---------------------
test/suites/api/test_pack.c | 95 ++++--
test/suites/api/test_unpack.c | 63 ++--
3 files changed, 491 insertions(+), 440 deletions(-)

diff --git a/src/variadic.c b/src/variadic.c
index 58974ee..69dad6f 100644
--- a/src/variadic.c
+++ b/src/variadic.c
@@ -13,87 +13,134 @@
#include <jansson.h>
#include "jansson_private.h"

-static json_t *json_vnpack(json_error_t *error, ssize_t size, const char *fmt, va_list ap)
+static json_t *json_vnpack(json_error_t *error, ssize_t size, const char * const fmt, va_list ap)
{
-
- /* Keep a stack of containers (lists and objects) */
- int depth = 0;
- json_t **stack = NULL;
-
- /* Keep a list of objects we create in case of error */
- int free_count = 0;
- json_t **free_list = NULL;
-
- json_t *cur = NULL; /* Current container */

json_t *root = NULL; /* root object */
json_t *obj = NULL;

+ /* Scanner variables */

const char *tok = fmt;

+ const char *etok;
+ int etok_depth;

char *key = NULL; /* Current key in an object */
char *s;

- int line = 1;
+ int line=1;
+ int column=1;
+
+ /* Skip whitespace at the beginning of the string. */
+ while(size && *tok == ' ') {
+ tok++;
+ size--;
+ column++;
+ }
+
+ if(size <= 0) {
+ jsonp_error_set(error, 1, 1, "Empty format string!");
+ return(NULL);
+ }
+
+ /* tok must contain either a container type, or a length-1 string for a
+ * simple type. */
+ if(*tok == '[')
+ root = json_array();
+ else if(*tok == '{')
+ root = json_object();
+ else
+ {
+ /* Simple object. Permit trailing spaces, otherwise complain. */
+ if((ssize_t)strspn(tok+1, " ") < size-1)
+ {
+ jsonp_error_set(error, 1, 1,
+ "Expected a single object, got %i", size);
+ return(NULL);
+ }
+
+ switch(*tok)

+ {
+ case 's': /* string */
+ s = va_arg(ap, char*);

+ if(!s)
+ {
+ jsonp_error_set(error, 1, 1,
+ "Refusing to handle a NULL string");
+ return(NULL);
+ }
+ return(json_string(s));

+
+ case 'n': /* null */

+ return(json_null());

+
+ case 'b': /* boolean */
+ obj = va_arg(ap, int) ?
+ json_true() : json_false();

+ return(obj);

+
+ case 'i': /* integer */

+ return(json_integer(va_arg(ap, int)));

+
+ case 'f': /* double-precision float */

+ return(json_real(va_arg(ap, double)));

+
+ case 'O': /* a json_t object; increments refcount */
+ obj = va_arg(ap, json_t *);
+ json_incref(obj);

+ return(obj);

+
+ case 'o': /* a json_t object; doesn't increment refcount */
+ obj = va_arg(ap, json_t *);

+ return(obj);

- /* Allocation provisioned for worst case */
- stack = calloc(size, sizeof(json_t *));
- free_list = calloc(size, sizeof(json_t *));
+ default: /* Whoops! */
+ jsonp_error_set(error, 1, 1,
+ "Didn't understand format character '%c'",
+ *tok);
+ return(NULL);
+ }
+ }

- if(!stack || !free_list)
- goto out;
+ /* Move past container opening token */
+ tok++;
+ column++;

while(tok-fmt < size) {
switch(*tok) {
case '\n':
line++;
+ column=0;
break;

case ' ': /* Whitespace */
break;

case ',': /* Element spacer */

- if(!root)
- {
- jsonp_error_set(error, line, tok-fmt,
- "Unexpected COMMA precedes root element!");
- root = NULL;
- goto out;
- }
-
- if(!cur)
- {
- jsonp_error_set(error, line, tok-fmt,
- "Unexpected COMMA outside a list or object!");
- root = NULL;
- goto out;
- }
-
if(key)
{
- jsonp_error_set(error, line, tok-fmt,
+ jsonp_error_set(error, line, column,

"Expected KEY, got COMMA!");

- root = NULL;
- goto out;
+ json_decref(root);
+ return(NULL);
}
break;

case ':': /* Key/value separator */
if(!key)
{

- jsonp_error_set(error, line, tok-fmt,
+ jsonp_error_set(error, line, column,

"Got key/value separator without "
"a key preceding it!");

- root = NULL;
- goto out;
+ json_decref(root);
+ return(NULL);
}

- if(!json_is_object(cur))
+ if(!json_is_object(root))
{
- jsonp_error_set(error, line, tok-fmt,
+ jsonp_error_set(error, line, column,

"Got a key/value separator "
"(':') outside an object!");

- root = NULL;
- goto out;
+ json_decref(root);
+ return(NULL);
}

break;
@@ -101,62 +148,85 @@ static json_t *json_vnpack(json_error_t *error, ssize_t size, const char *fmt, v

case ']': /* Close array or object */

case '}':

- if(key)
+ if(tok-fmt + (ssize_t)strspn(tok+1, " ") != size-1)
{
- jsonp_error_set(error, line, tok-fmt,
- "OBJECT or ARRAY ended with an "
- "incomplete key/value pair!");
- root = NULL;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Unexpected close-bracket '%c'", *tok);
+ json_decref(root);
+ return(NULL);
}

- if(depth <= 0)
+ if((*tok == ']' && !json_is_array(root)) ||
+ (*tok == '}' && !json_is_object(root)))
{
- jsonp_error_set(error, line, tok-fmt,
- "Too many close-brackets '%c'", *tok);
- root = NULL;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Stray close-array '%c' character", *tok);
+ json_decref(root);
+ return(NULL);
}
+ return(root);

- if(*tok == ']' && !json_is_array(cur))
- {
- jsonp_error_set(error, line, tok-fmt,
- "Stray close-array ']' character");
- root = NULL;
- goto out;
- }
+ case '[':
+ case '{':

- if(*tok == '}' && !json_is_object(cur))
- {
- jsonp_error_set(error, line, tok-fmt,
- "Stray close-object '}' character");
- root = NULL;
- goto out;
- }
+ /* Shortcut so we don't mess up the column count in error
+ * messages */
+ if(json_is_object(root) && !key)
+ goto common;
+
+ /* Find corresponding close bracket */
+ etok = tok+1;
+ etok_depth = 1;
+ while(etok_depth) {
+
+ if(!*etok || etok-fmt >= size) {
+ jsonp_error_set(error, line, column,
+ "Couldn't find matching close bracket for '%c'",
+ *tok);
+ json_decref(root);
+ return(NULL);
+ }

- cur = stack[--depth];
- break;
+ if(*tok==*etok)
+ etok_depth++;
+ else if(*tok=='[' && *etok==']') {
+ etok_depth--;
+ break;
+ } else if(*tok=='{' && *etok=='}') {
+ etok_depth--;
+ break;
+ }

- case '[':
- obj = json_array();
- goto obj_common;
+ etok++;
+ }

- case '{':
- obj = json_object();
- goto obj_common;
+ /* Recurse */
+ obj = json_vnpack(error, etok-tok+1, tok, ap);
+ if(!obj) {
+ /* error should already be set */
+ error->column += column-1;
+ error->line += line-1;
+ json_decref(root);
+ return(NULL);
+ }
+ column += etok-tok;
+ tok = etok;
+ goto common;

- case 's': /* string */
+ case 's':
+ /* Handle strings specially, since they're used for both keys
+ * and values */
s = va_arg(ap, char*);

if(!s)
{
- jsonp_error_set(error, line, tok-fmt,
+ jsonp_error_set(error, line, column,

"Refusing to handle a NULL string");

- root = NULL;
- goto out;
+ json_decref(root);
+ return(NULL);
}

- if(json_is_object(cur) && !key)
+ if(json_is_object(root) && !key)
{

/* It's a key */

key = s;
@@ -164,99 +234,43 @@ static json_t *json_vnpack(json_error_t *error, ssize_t size, const char *fmt, v
}

obj = json_string(s);
- goto obj_common;
-
- case 'n': /* null */
- obj = json_null();
- goto obj_common;
-
- case 'b': /* boolean */
- obj = va_arg(ap, int) ?
- json_true() : json_false();
- goto obj_common;
-
- case 'i': /* integer */
- obj = json_integer(va_arg(ap, int));
- goto obj_common;
-
- case 'f': /* double-precision float */
- obj = json_real(va_arg(ap, double));
- goto obj_common;
-
- case 'O': /* a json_t object; increments refcount */
- obj = va_arg(ap, json_t *);
- json_incref(obj);
- goto obj_common;
+ goto common;

- case 'o': /* a json_t object; doesn't increment refcount */
- obj = va_arg(ap, json_t *);
- goto obj_common;
-
-obj_common: free_list[free_count++] = obj;
+ default:
+ obj = json_vnpack(error, 1, tok, ap);
+ if(!obj) {
+ json_decref(root);
+ return(NULL);
+ }

- /* Root this object to its parent */
- if(json_is_object(cur)) {
+common:
+ /* Add to container */
+ if(json_is_object(root)) {
if(!key)
{
- jsonp_error_set(error, line, tok-fmt,
+ jsonp_error_set(error, line, column,

"Expected key, got identifier '%c'!", *tok);

- root = NULL;
- goto out;
+ json_decref(root);
+ return(NULL);
}

- json_object_set_new(cur, key, obj);
+ json_object_set_new(root, key, obj);
key = NULL;
}
- else if(json_is_array(cur))
- {
- json_array_append_new(cur, obj);
- }
- else if(!root)
- {
- root = obj;
- }
else
{
- jsonp_error_set(error, line, tok-fmt,
- "Can't figure out where to attach "
- "'%c' object!", *tok);
- root = NULL;
- goto out;
+ json_array_append_new(root, obj);
}
-
- /* If it was a container ('[' or '{'), descend on the stack */
- if(json_is_array(obj) || json_is_object(obj))
- {
- stack[depth++] = cur;
- cur = obj;
- }
-
break;
}
tok++;
+ column++;
}

- if(depth != 0) {
- jsonp_error_set(error, line, tok-fmt,
- "Missing object or array close-brackets in format string");
- root = NULL;
- goto out;
- }
-
- /* Success: don't free everything we just built! */
- free_count = 0;
-
-out:
- while(free_count)
- json_decref(free_list[--free_count]);
-
- if(free_list)
- free(free_list);
-
- if(stack)
- free(stack);
-
- return(root);
+ /* Whoops -- we didn't match the close bracket! */
+ jsonp_error_set(error, line, column, "Missing close array or object!");
+ json_decref(root);
+ return(NULL);

}

static int json_vnunpack(json_t *root, json_error_t *error, ssize_t size, const char *fmt, va_list ap)

@@ -264,307 +278,280 @@ static int json_vnunpack(json_t *root, json_error_t *error, ssize_t size, const

int rv=0; /* Return value */
int line = 1; /* Line number */

+ int column = 1; /* Column */

- /* Keep a stack of containers (lists and objects) */
- int depth = 0;
- json_t **stack = NULL;
-
+ /* Position markers for arrays or objects */
int array_index = 0;
- char *key = NULL; /* Current key in an object */

+ char *key = NULL;

- json_t *cur = NULL; /* Current container */
- json_t *obj = NULL;
+ const char **s;

+ /* Scanner variables */

const char *tok = fmt;

+ const char *etok;
+ int etok_depth;

- /* Allocation provisioned for worst case */
- stack = calloc(size, sizeof(json_t *));
- if(!stack)
- {
- jsonp_error_set(error, line, 0, "Out of memory!");
- rv = -1;
- goto out;
- }
+ json_t *obj;

- /* Even if we're successful, we need to know if the number of
- * arguments provided matches the number of JSON objects.
- * We can do this by counting the elements in every array or
- * object we open up, and decrementing the count as we visit
- * their children. */
+ /* If we're successful, we need to know if the number of arguments
+ * provided matches the number of JSON objects. We can do this by
+ * counting the elements in every array or object we open up, and
+ * decrementing the count as we visit their children. */
int unvisited = 0;

- while(tok-fmt < size)
- {
- switch(*tok)
- {
- case ' ': /* Whitespace */
- break;
+ /* Skip whitespace at the beginning of the string. */
+ while(size && *tok == ' ') {
+ tok++;
+ size--;
+ column++;
+ }

- case '\n': /* Line break */
- line++;
- break;
+ if(size <= 0) {
+ jsonp_error_set(error, 1, 1, "Empty format string!");
+ return(-2);
+ }

- case ',': /* Element spacer */
+ /* tok must contain either a container type, or a length-1 string for a
+ * simple type. */
+ if(*tok != '[' && *tok != '{')
+ {
+ /* Simple object. Permit trailing spaces, otherwise complain. */
+ if((ssize_t)strspn(tok+1, " ") < size-1)
+ {
+ jsonp_error_set(error, 1, 1,
+ "Expected a single object, got %i", size);
+ return(-1);
+ }

- if(!cur)
+ switch(*tok)
+ {
+ case 's':
+ if(!json_is_string(root))
{
- jsonp_error_set(error, line, tok-fmt,
- "Unexpected COMMA outside a list or object!");
- rv = -1;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Type mismatch! Object (%i) wasn't a string.",
+ json_typeof(root));
+ return(-2);
}
-
- if(key)
- {
- jsonp_error_set(error, line, tok-fmt,
- "Expected KEY, got COMMA!");
- rv = -1;
- goto out;
+ s = va_arg(ap, const char **);
+ if(!s) {
+ jsonp_error_set(error, line, column, "Passed a NULL string pointer!");
+ return(-2);
}
- break;
+ *s = json_string_value(root);
+ return(0);

- case ':': /* Key/value separator */
- if(!json_is_object(cur) || !key)
+ case 'i':
+ if(!json_is_integer(root))
{
- jsonp_error_set(error, line, tok-fmt, "Unexpected ':'");
- rv = -1;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Type mismatch! Object (%i) wasn't an integer.",
+ json_typeof(root));
+ return(-2);
}
- break;
+ *va_arg(ap, int*) = json_integer_value(root);
+ return(0);

- case '[':
- case '{':
- /* Fetch object */
- if(!cur)
+ case 'b':
+ if(!json_is_boolean(root))
{
- obj = root;
- }
- else if(json_is_object(cur))
- {
- if(!key)
- {
- jsonp_error_set(error, line, tok-fmt,
- "Objects can't be keys");
- rv = -1;
- goto out;
- }
- obj = json_object_get(cur, key);
- unvisited--;
- key = NULL;
+ jsonp_error_set(error, line, column,
+ "Type mismatch! Object (%i) wasn't a boolean.",
+ json_typeof(root));
+ return(-2);
}
- else if(json_is_array(cur))
+ *va_arg(ap, int*) = json_is_true(root);
+ return(0);
+
+ case 'f':
+ if(!json_is_number(root))
{
- obj = json_array_get(cur, array_index);
- unvisited--;
- array_index++;
+ jsonp_error_set(error, line, column,
+ "Type mismatch! Object (%i) wasn't a real.",
+ json_typeof(root));
+ return(-2);
}
- else
+ *va_arg(ap, double*) = json_number_value(root);
+ return(0);
+
+ case 'O':
+ json_incref(root);

+ /* Fall through */
+
+ case 'o':

+ *va_arg(ap, json_t**) = root;
+ return(0);

+
+ case 'n':
+ /* Don't actually assign anything; we're just happy
+ * the null turned up as promised in the format
+ * string. */

+ return(0);
+
+ default:
+ jsonp_error_set(error, line, column,

+ "Unknown format character '%c'", *tok);

+ return(-1);
+ }
+ }
+
+ /* Move past container opening token */
+ tok++;
+

+ while(tok-fmt < size) {
+ switch(*tok) {

+ case '\n':
+ line++;

+ column=0;

+ break;
+
+ case ' ': /* Whitespace */
+ break;
+

+ case ',': /* Element spacer */
+ if(key)
{
- assert(0);
+ jsonp_error_set(error, line, column,
+ "Expected KEY, got COMMA!");
+ return(-2);
}
+ break;

- /* Make sure we got what we expected */
- if(*tok=='{' && !json_is_object(obj))
+ case ':': /* Key/value separator */
+ if(!key)
{
- jsonp_error_set(error, line, tok-fmt,
- "Token '{' didn't correspond to an object");
- rv = -2;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Got key/value separator without "
+ "a key preceding it!");
+ return(-2);
}

- if(*tok=='[' && !json_is_array(obj))
+ if(!json_is_object(root))
{
- jsonp_error_set(error, line, tok-fmt,
- "Token '[' didn't correspond to an object");
- rv = -2;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Got a key/value separator "
+ "(':') outside an object!");
+ return(-2);
}

- unvisited += json_is_object(obj) ?
- json_object_size(obj) :
- json_array_size(obj);
-
- /* Descend */
- stack[depth++] = cur;
- cur = obj;
-
- key = NULL;
-
break;

-
- case ']':

+ case ']': /* Close array or object */

case '}':

- if(json_is_array(cur) && *tok!=']')
+ if(tok-fmt + (ssize_t)strspn(tok+1, " ") != size-1)
{
- jsonp_error_set(error, line, tok-fmt, "Missing ']'");
- rv = -1;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Unexpected close-bracket '%c'", *tok);
+ return(-2);
}

- if(json_is_object(cur) && *tok!='}')
+ if((*tok == ']' && !json_is_array(root)) ||
+ (*tok == '}' && !json_is_object(root)))
{
- jsonp_error_set(error, line, tok-fmt, "Missing '}'");
- rv = -1;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Stray close-array '%c' character", *tok);
+ return(-2);
}
+ return(unvisited);

- if(key)
- {
- jsonp_error_set(error, line, tok-fmt, "Unexpected '%c'", *tok);
- rv = -1;
- goto out;

+ case '[':
+ case '{':
+

+ /* Find corresponding close bracket */
+ etok = tok+1;
+ etok_depth = 1;
+ while(etok_depth) {
+
+ if(!*etok || etok-fmt >= size) {
+ jsonp_error_set(error, line, column,
+ "Couldn't find matching close bracket for '%c'",
+ *tok);
+ return(-2);
+ }
+
+ if(*tok==*etok)
+ etok_depth++;
+ else if(*tok=='[' && *etok==']') {
+ etok_depth--;
+ break;
+ } else if(*tok=='{' && *etok=='}') {
+ etok_depth--;
+ break;
+ }
+
+ etok++;
}

- if(depth <= 0)
- {
- jsonp_error_set(error, line, tok-fmt, "Unexpected '%c'", *tok);
- rv = -1;
- goto out;
+ /* Recurse */
+ if(json_is_array(root)) {
+ rv = json_vnunpack(json_object_get(root, key),
+ error, etok-tok+1, tok, ap);
+ } else {
+ rv = json_vnunpack(json_array_get(root, array_index++),
+ error, etok-tok+1, tok, ap);
}

- cur = stack[--depth];
+ if(rv < 0) {
+ /* error should already be set */
+ error->column += column-1;
+ error->line += line-1;
+ return(rv);
+ }

+ unvisited += rv;
+ column += etok-tok;
+ tok = etok;
break;

case 's':
- if(!key && json_is_object(cur))
+ /* Handle strings specially, since they're used for both keys
+ * and values */
+
+ if(json_is_object(root) && !key)
{
- /* constant string for key */

+ /* It's a key */

key = va_arg(ap, char*);
- break;
- }
- /* fall through */
-
- case 'i': /* integer */
- case 'f': /* double-precision float */
- case 'O': /* a json_t object; increments refcount */
- case 'o': /* a json_t object; borrowed reference */
- case 'b': /* boolean */
- case 'n': /* null */
+ printf("Got key '%s'\n", key);

- /* Fetch object */
- if(!cur)
- {
- obj = root;
- }
- else if(json_is_object(cur))
- {
if(!key)
{
- jsonp_error_set(error, line, tok-fmt,
- "Only strings may be used as keys!");
- rv = -1;
- goto out;
+ jsonp_error_set(error, line, column,
+ "Refusing to handle a NULL key");
+ return(-2);
}
-
- obj = json_object_get(cur, key);
- unvisited--;
- key = NULL;
- }
- else if(json_is_array(cur))
- {
- obj = json_array_get(cur, array_index);
- unvisited--;
- array_index++;
- }
- else
- {
- jsonp_error_set(error, line, tok-fmt,
- "Unsure how to retrieve JSON object '%c'",
- *tok);
- rv = -1;
- goto out;
+ break;
}

- switch(*tok)
- {
- case 's':
- if(!json_is_string(obj))
- {
- jsonp_error_set(error, line, tok-fmt,
- "Type mismatch! Object wasn't a string.");
- rv = -2;
- goto out;
- }
- *va_arg(ap, const char**) = json_string_value(obj);
- break;

+ /* Fall through */

- case 'i':
- if(!json_is_integer(obj))
- {
- jsonp_error_set(error, line, tok-fmt,
- "Type mismatch! Object wasn't an integer.");
- rv = -2;
- goto out;
- }
- *va_arg(ap, int*) = json_integer_value(obj);
- break;
+ default:

- case 'b':
- if(!json_is_boolean(obj))
- {
- jsonp_error_set(error, line, tok-fmt,
- "Type mismatch! Object wasn't a boolean.");
- rv = -2;
- goto out;
- }
- *va_arg(ap, int*) = json_is_true(obj);
- break;
-
- case 'f':
- if(!json_is_number(obj))
- {
- jsonp_error_set(error, line, tok-fmt,
- "Type mismatch! Object wasn't a real.");
- rv = -2;
- goto out;
- }
- *va_arg(ap, double*) = json_number_value(obj);
- break;
+ /* Fetch the element from the JSON container */
+ if(json_is_object(root))
+ obj = json_object_get(root, key);
+ else
+ obj = json_array_get(root, array_index++);

- case 'O':
- json_incref(obj);
- /* Fall through */
+ if(!obj) {
+ jsonp_error_set(error, line, column,
+ "Array/object entry didn't exist!");
+ return(-1);
+ }

- case 'o':
- *va_arg(ap, json_t**) = obj;
- break;
+ rv = json_vnunpack(obj, error, 1, tok, ap);
+ if(rv != 0)
+ return(rv);

- case 'n':
- /* Don't actually assign anything; we're just happy
- * the null turned up as promised in the format
- * string. */
- break;
-
- default:
- jsonp_error_set(error, line, tok-fmt,
- "Unknown format character '%c'", *tok);
- rv = -1;
- goto out;
- }
+ break;
}
tok++;
+ column++;
}

- if(depth != 0) {
- jsonp_error_set(error, line, tok-fmt,
- "Missing object or array close-brackets in format string");
- rv = -2;
- goto out;
- }
-
- /* Return 0 if everything was matched; otherwise the number of JSON
- * objects we didn't get to. */
- rv = unvisited;
-
-out:
-
- if(stack)
- free(stack);
-
- return(rv);
+ /* Whoops -- we didn't match the close bracket! */
+ jsonp_error_set(error, line, column, "Missing close array or object!");
+ return(-2);

}

json_t *json_pack(json_error_t *error, const char *fmt, ...)

@@ -575,7 +562,7 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...)
jsonp_error_init(error, "");

if(!fmt || !*fmt) {
- jsonp_error_set(error, 1, 0, "Null or empty format string!");
+ jsonp_error_set(error, 1, 1, "Null or empty format string!");
return(NULL);
}

@@ -594,7 +581,7 @@ int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...)
jsonp_error_init(error, "");

if(!fmt || !*fmt) {
- jsonp_error_set(error, 1, 0, "Null or empty format string!");
+ jsonp_error_set(error, 1, 1, "Null or empty format string!");
return(-2);;
}

diff --git a/test/suites/api/test_pack.c b/test/suites/api/test_pack.c
index f1ca388..8968dc7 100644
--- a/test/suites/api/test_pack.c
+++ b/test/suites/api/test_pack.c
@@ -15,13 +15,14 @@ int main()
{
json_t *value;
int i;
+ json_error_t error;

/*

* Simple, valid json_pack cases

*/

/* true */
- value = json_pack(NULL, "b", 1);
+ value = json_pack(&error, "b", 1);
if(!json_is_true(value))

fail("json_pack boolean failed");

if(value->refcount != (ssize_t)-1)
@@ -29,7 +30,7 @@ int main()
json_decref(value);

/* false */
- value = json_pack(NULL, "b", 0);
+ value = json_pack(&error, "b", 0);
if(!json_is_false(value))

fail("json_pack boolean failed");

if(value->refcount != (ssize_t)-1)
@@ -37,7 +38,7 @@ int main()
json_decref(value);

/* null */
- value = json_pack(NULL, "n");
+ value = json_pack(&error, "n");
if(!json_is_null(value))

fail("json_pack null failed");

if(value->refcount != (ssize_t)-1)
@@ -45,7 +46,7 @@ int main()
json_decref(value);

/* integer */
- value = json_pack(NULL, "i", 1);
+ value = json_pack(&error, "i", 1);
if(!json_is_integer(value) || json_integer_value(value) != 1)

fail("json_pack integer failed");

if(value->refcount != (ssize_t)1)
@@ -54,7 +55,7 @@ int main()


/* real */
- value = json_pack(NULL, "f", 1.0);
+ value = json_pack(&error, "f", 1.0);
if(!json_is_real(value) || json_real_value(value) != 1.0)

fail("json_pack real failed");

if(value->refcount != (ssize_t)1)
@@ -62,7 +63,7 @@ int main()
json_decref(value);

/* string */
- value = json_pack(NULL, "s", "test");
+ value = json_pack(&error, "s", "test");

if(!json_is_string(value) || strcmp("test", json_string_value(value)))

fail("json_pack string failed");

if(value->refcount != (ssize_t)1)
@@ -70,7 +71,7 @@ int main()
json_decref(value);

/* empty object */
- value = json_pack(NULL, "{}", 1.0);
+ value = json_pack(&error, "{}", 1.0);
if(!json_is_object(value) || json_object_size(value) != 0)

fail("json_pack empty object failed");

if(value->refcount != (ssize_t)1)
@@ -78,7 +79,7 @@ int main()
json_decref(value);

/* empty list */
- value = json_pack(NULL, "[]", 1.0);
+ value = json_pack(&error, "[]", 1.0);
if(!json_is_array(value) || json_array_size(value) != 0)

fail("json_pack empty list failed");

if(value->refcount != (ssize_t)1)
@@ -86,7 +87,7 @@ int main()
json_decref(value);

/* non-incref'd object */

- value = json_pack(NULL, "o", json_integer(1));
+ value = json_pack(&error, "o", json_integer(1));
if(!json_is_integer(value) || json_integer_value(value) != 1)

fail("json_pack object failed");

if(value->refcount != (ssize_t)1)
@@ -94,7 +95,7 @@ int main()
json_decref(value);

/* incref'd object */

- value = json_pack(NULL, "O", json_integer(1));
+ value = json_pack(&error, "O", json_integer(1));
if(!json_is_integer(value) || json_integer_value(value) != 1)

fail("json_pack object failed");

if(value->refcount != (ssize_t)2)
@@ -103,17 +104,17 @@ int main()
json_decref(value);

/* simple object */
- value = json_pack(NULL, "{s:[]}", "foo");
+ value = json_pack(&error, "{s:[]}", "foo");
if(!json_is_object(value) || json_object_size(value) != 1)
- fail("json_pack object failed");
+ fail("json_pack array failed");
if(!json_is_array(json_object_get(value, "foo")))
- fail("json_pack object failed");
+ fail("json_pack array failed");

if(json_object_get(value, "foo")->refcount != (ssize_t)1)

fail("json_pack object refcount failed");

json_decref(value);

/* simple array */
- value = json_pack(NULL, "[i,i,i]", 0, 1, 2);
+ value = json_pack(&error, "[i,i,i]", 0, 1, 2);
if(!json_is_array(value) || json_array_size(value) != 3)

fail("json_pack object failed");

for(i=0; i<3; i++)
@@ -125,30 +126,82 @@ int main()
}
json_decref(value);

+ /* Whitespace; regular string */
+ value = json_pack(&error, " s ", "test");

+ if(!json_is_string(value) || strcmp("test", json_string_value(value)))

+ fail("json_pack string (with whitespace) failed");
+ json_decref(value);
+
+ /* Whitespace; empty array */
+ value = json_pack(&error, "[ ]");

+ if(!json_is_array(value) || json_array_size(value) != 0)

+ fail("json_pack empty array (with whitespace) failed");
+ json_decref(value);
+
+ /* Whitespace; array */
+ value = json_pack(&error, "[ i , i, i ] ", 1, 2, 3);

+ if(!json_is_array(value) || json_array_size(value) != 3)

+ fail("json_pack array (with whitespace) failed");
+ json_decref(value);
+
/*
* Invalid cases
*/
-

+
/* mismatched open/close array/object */

- if(json_pack(NULL, "[}"))
+ if(json_pack(&error, "[}"))
fail("json_pack failed to catch mismatched '}'");
+ if(error.line != 1 || error.column != 2)
+ fail("json_pack didn't get the error coordinates right!");

- if(json_pack(NULL, "{]"))
+ if(json_pack(&error, "{]"))
fail("json_pack failed to catch mismatched ']'");
+ if(error.line != 1 || error.column != 2)
+ fail("json_pack didn't get the error coordinates right!");

/* missing close array */

- if(json_pack(NULL, "["))
+ if(json_pack(&error, "["))
fail("json_pack failed to catch missing ']'");
+ if(error.line != 1 || error.column != 2)
+ fail("json_pack didn't get the error coordinates right!");

/* missing close object */

- if(json_pack(NULL, "{"))
+ if(json_pack(&error, "{"))
fail("json_pack failed to catch missing '}'");
+ if(error.line != 1 || error.column != 2)
+ fail("json_pack didn't get the error coordinates right!");

/* NULL string */
- if(json_pack(NULL, "s", NULL))
- fail("json_pack failed to catch null string");
+ if(json_pack(&error, "s", NULL))
+ fail("json_pack failed to catch null argument string");
+ if(error.line != 1 || error.column != 1)
+ fail("json_pack didn't get the error coordinates right!");
+
+ /* NULL format */
+ if(json_pack(&error, NULL))
+ fail("json_pack failed to catch NULL format string");
+ if(error.line != 1 || error.column != 1)
+ fail("json_pack didn't get the error coordinates right!");
+
+ /* More complicated checks for row/columns */
+ if(json_pack(&error, "{ {}: s }", "foo"))
+ fail("json_pack failed to catch object as key");
+ if(error.line != 1 || error.column != 3)
+ fail("json_pack didn't get the error coordinates right!");
+
+ if(json_pack(&error, "{ s: {}, s:[ii{} }", "foo", "bar", 12, 13))
+ fail("json_pack failed to catch missing ]");
+ if(error.line != 1 || error.column != 13)
+ fail("json_pack didn't get the error coordinates right!");
+
+ if(json_pack(&error, "[[[[[ [[[[[ [[[[ }]]]] ]]]] ]]]]]"))
+ fail("json_pack failed to catch missing ]");
+ if(error.line != 1 || error.column != 21)
+ fail("json_pack didn't get the error coordinates right!");

return(0);
+
+ //fprintf(stderr, "%i/%i: %s %s\n", error.line, error.column, error.source, error.text);

}

/* vim: ts=4:expandtab:sw=4

diff --git a/test/suites/api/test_unpack.c b/test/suites/api/test_unpack.c
index 933ed2f..6bc2e66 100644
--- a/test/suites/api/test_unpack.c
+++ b/test/suites/api/test_unpack.c
@@ -20,85 +20,87 @@ int main()
double f;
char *s;

+ json_error_t error;
+
/*

* Simple, valid json_pack cases

*/

/* true */
- rv = json_unpack(json_true(), NULL, "b", &i1);
+ rv = json_unpack(json_true(), &error, "b", &i1);
if(rv || !i1)

fail("json_unpack boolean failed");

/* false */
- rv = json_unpack(json_false(), NULL, "b", &i1);
+ rv = json_unpack(json_false(), &error, "b", &i1);
if(rv || i1)

fail("json_unpack boolean failed");

/* null */
- rv = json_unpack(json_null(), NULL, "n");
+ rv = json_unpack(json_null(), &error, "n");
if(rv)

fail("json_unpack null failed");

/* integer */
j = json_integer(1);
- rv = json_unpack(j, NULL, "i", &i1);
+ rv = json_unpack(j, &error, "i", &i1);
if(rv || i1 != 1)

fail("json_unpack integer failed");

json_decref(j);

/* real */
j = json_real(1.0);
- rv = json_unpack(j, NULL, "f", &f);
+ rv = json_unpack(j, &error, "f", &f);
if(rv || f != 1.0)

fail("json_unpack real failed");

json_decref(j);

/* string */

j = json_string("foo");

- rv = json_unpack(j, NULL, "s", &s);
+ rv = json_unpack(j, &error, "s", &s);
if(rv || strcmp(s, "foo"))

fail("json_unpack string failed");

json_decref(j);

/* empty object */
j = json_object();
- rv = json_unpack(j, NULL, "{}");
+ rv = json_unpack(j, &error, "{}");
if(rv)

fail("json_unpack empty object failed");

json_decref(j);

/* empty list */
j = json_array();
- rv = json_unpack(j, NULL, "[]");
+ rv = json_unpack(j, &error, "[]");
if(rv)

fail("json_unpack empty list failed");

json_decref(j);

/* non-incref'd object */

j = json_object();
- rv = json_unpack(j, NULL, "o", &j2);
+ rv = json_unpack(j, &error, "o", &j2);

if(j2 != j || j->refcount != (ssize_t)1)

fail("json_unpack object failed");

json_decref(j);

/* incref'd object */

j = json_object();
- rv = json_unpack(j, NULL, "O", &j2);
+ rv = json_unpack(j, &error, "O", &j2);

if(j2 != j || j->refcount != (ssize_t)2)

fail("json_unpack object failed");

json_decref(j);
json_decref(j);

/* simple object */
- j = json_pack(NULL, "{s:i}", "foo", 1);
- rv = json_unpack(j, NULL, "{s:i}", "foo", &i1);
+ j = json_pack(&error, "{s:i}", "foo", 1);
+ rv = json_unpack(j, &error, "{s:i}", "foo", &i1);
if(rv || i1!=1)

fail("json_unpack simple object failed");

json_decref(j);

/* simple array */
- j = json_pack(NULL, "[iii]", 1, 2, 3);
- rv = json_unpack(j, NULL, "[i,i,i]", &i1, &i2, &i3);
+ j = json_pack(&error, "[iii]", 1, 2, 3);
+ rv = json_unpack(j, &error, "[i,i,i]", &i1, &i2, &i3);

if(rv || i1 != 1 || i2 != 2 || i3 != 3)

fail("json_unpack simple array failed");
json_decref(j);

@@ -108,40 +110,49 @@ int main()
*/

/* mismatched open/close array/object */

- j = json_pack(NULL, "[]");
- rv = json_unpack(j, NULL, "[}");
+ j = json_pack(&error, "[]");
+ rv = json_unpack(j, &error, "[}");
if(!rv)

fail("json_unpack failed to catch mismatched ']'");

json_decref(j);

- j = json_pack(NULL, "{}");
- rv = json_unpack(j, NULL, "{]");
+ j = json_pack(&error, "{}");
+ rv = json_unpack(j, &error, "{]");
if(!rv)

fail("json_unpack failed to catch mismatched '}'");

json_decref(j);

/* missing close array */

- j = json_pack(NULL, "[]");
- rv = json_unpack(j, NULL, "[");
+ j = json_pack(&error, "[]");
+ rv = json_unpack(j, &error, "[");
if(rv >= 0)

fail("json_unpack failed to catch missing ']'");

json_decref(j);

/* missing close object */

- j = json_pack(NULL, "{}");
- rv = json_unpack(j, NULL, "{");
+ j = json_pack(&error, "{}");
+ rv = json_unpack(j, &error, "{");
if(rv >= 0)

fail("json_unpack failed to catch missing '}'");

json_decref(j);

- /* NULL string */
- j = json_pack(NULL, "[]");
- rv =json_unpack(j, NULL, NULL);
+ /* NULL format string */
+ j = json_pack(&error, "[]");
+ rv =json_unpack(j, &error, NULL);
if(rv >= 0)
- fail("json_unpack failed to catch null string");
+ fail("json_unpack failed to catch null format string");
+ json_decref(j);
+
+ /* NULL string pointer */
+ j = json_string("foobie");
+ rv =json_unpack(j, &error, "s", NULL);
+ if(rv >= 0)
+ fail("json_unpack failed to catch null string pointer");
json_decref(j);

return 0;
+
+ //fprintf(stderr, "%i/%i: %s %s\n", error.line, error.column, error.source, error.text);

}

/* vim: ts=4:expandtab:sw=4

--
1.7.2.3

[Petri Lehtinen]

gsme...@gmail.com wrote:
> From: Graeme Smecher <graeme....@mail.mcgill.ca>
>
> This change will facilitate the use of a recursive-descent parser for both
> json_pack and json_unpack. We also add some error-checking, implement the
> new json_error_t "column" field, and remove a debugging printf that snuck into
> an earlier commit.

Thanks. The code looks good in general. However, I ran the tests and
both pack and unpack tests seem to fail at one point:

sh> Suite: api
sh> ........F.F
sh> =================================================================
sh> api/test_pack.c
sh> =================================================================
sh> test_pack.c:main:125: json_pack integer array failed
sh>
sh> =================================================================
sh> api/test_unpack.c
sh> =================================================================
sh> test_unpack.c:main:105: json_unpack simple array failed
sh>
sh> =================================================================

Do they pass for you?

[Graeme Smecher]

Hi Petri,

Hm... They did. I must have dropped a patch or something while cleaning
up the commits.

I'll have a look tomorrow and send along a revised version.

best,
Graeme

[Graeme Smecher]

Hi Petri,

On 07/11/10 03:32 AM, Petri Lehtinen wrote:

sh> Suite: api
sh> ........F.F
sh> =================================================================
sh> api/test_pack.c
sh> =================================================================
sh> test_pack.c:main:125: json_pack integer array failed
sh> 
sh> =================================================================
sh> api/test_unpack.c
sh> =================================================================
sh> test_unpack.c:main:105: json_unpack simple array failed
sh> 
sh> =================================================================

Do they pass for you?
  

Actually, they do... I just tried the following:

    * clone a fresh jansson tree,
    * apply those three patches,
    * make check

...and I get a successful result:

Suite: api
...........
Suite: encoding-flags
...........
Suite: invalid
...............................................
Suite: invalid-strip
...............................................
Suite: invalid-unicode
..................
Suite: valid
.................................
Suite: valid-strip
.................................
7 test suites passed
PASS: run-suites
=============
1 test passed
=============

Valgrind doesn't complain either. It could be a word-length problem, and I don't actually have a 32-bit box to test it on.

Can you doublecheck things at your end? I'll spend a little more time trying to reproduce the problem tomorrow. I notice that a debugging printf snuck into those patches (variadic.c:515), so I need to reformat them anyway.

cheers,
Graeme

[Petri Lehtinen]

I did a fresh checkout just like you and got the same errors. After
investigating, I noticed that the call va_arg(ap, int); returned 0
each time. I put loops using va_arg() here and there and at some
points it was returning the correct 1, 2, 3 sequence. I then checked
the stdarg manpage, here's a quote:

If ap is passed to a function that uses va_arg(ap,type) then the
value of ap is undefined after the return of that function.

So, it seems that the recursive approach is not the way to go after
all :(