From 7f3018a4fb547a10ce250db91ec0d41b0cc57bf2 Mon Sep 17 00:00:00 2001
From: Petri Lehtinen <petri@digip.org>
Date: Sun, 23 Jan 2011 21:14:19 +0200
Subject: [PATCH] Refactor json_pack()

* Implement a "scanner" that reads the format string, maintaining state

* Split json_vnpack() to three separate functions for packing objects,
  arrays and simple values. This makes it more clear what is being
  packed, and the object and array structures become more evident.

* Make the skipping of ignored character simpler, i.e. skip ':' and
  ',' independent of their context

This patch shaves around 80 lines of code from the original
implementation.
---
 src/error.c                 |  12 +-
 src/jansson_private.h       |   3 +
 src/variadic.c              | 364 +++++++++++++++++---------------------------
 test/suites/api/test_pack.c |   2 +-
 4 files changed, 152 insertions(+), 229 deletions(-)

diff --git a/src/error.c b/src/error.c
index b8c78db..cd80756 100644
--- a/src/error.c
+++ b/src/error.c
@@ -1,6 +1,4 @@
 #include <string.h>
-#include <stdarg.h>
-
 #include "jansson_private.h"
 
 void jsonp_error_init(json_error_t *error, const char *source)
@@ -21,6 +19,14 @@ void jsonp_error_set(json_error_t *error, int line, int column,
 {
     va_list ap;
 
+    va_start(ap, msg);
+    jsonp_error_vset(error, line, column, msg, ap);
+    va_end(ap);
+}
+
+void jsonp_error_vset(json_error_t *error, int line, int column,
+                      const char *msg, va_list ap)
+{
     if(!error)
         return;
 
@@ -32,7 +38,5 @@ void jsonp_error_set(json_error_t *error, int line, int column,
     error->line = line;
     error->column = column;
 
-    va_start(ap, msg);
     vsnprintf(error->text, JSON_ERROR_TEXT_LENGTH, msg, ap);
-    va_end(ap);
 }
diff --git a/src/jansson_private.h b/src/jansson_private.h
index f713d21..66550da 100644
--- a/src/jansson_private.h
+++ b/src/jansson_private.h
@@ -9,6 +9,7 @@
 #define JANSSON_PRIVATE_H
 
 #include <stddef.h>
+#include <stdarg.h>
 #include "jansson.h"
 #include "hashtable.h"
 
@@ -66,5 +67,7 @@ const object_key_t *jsonp_object_iter_fullkey(void *iter);
 void jsonp_error_init(json_error_t *error, const char *source);
 void jsonp_error_set(json_error_t *error, int line, int column,
                      const char *msg, ...);
+void jsonp_error_vset(json_error_t *error, int line, int column,
+                      const char *msg, va_list ap);
 
 #endif
diff --git a/src/variadic.c b/src/variadic.c
index 69af7e5..3f02a19 100644
--- a/src/variadic.c
+++ b/src/variadic.c
@@ -13,264 +13,166 @@
 #include <jansson.h>
 #include "jansson_private.h"
 
-static json_t *json_vnpack(json_error_t *error, ssize_t size, const char * const fmt, va_list *ap)
+typedef struct {
+    const char *fmt;
+    char token;
+    json_error_t *error;
+    int line;
+    int column;
+} scanner_t;
+
+static void next_token(scanner_t *s)
 {
-    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 column = 1;
+    const char *t = s->fmt;
+    s->column++;
+
+    /* skip space and ignored chars */
+    while(*t == ' ' || *t == '\t' || *t == '\n' || *t == ',' || *t == ':') {
+        if(*t == '\n') {
+            s->line++;
+            s->column = 1;
+        }
+        else
+            s->column++;
 
-    /* Skip whitespace at the beginning of the string. */
-    while(size && *tok == ' ') {
-        tok++;
-        size--;
-        column++;
+        t++;
     }
 
-    if(size <= 0) {
-        jsonp_error_set(error, 1, 1, "Empty format string!");
-        return NULL;
-    }
+    s->token = *t;
 
-    /* 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);
+    t++;
+    s->fmt = t;
+}
 
-            case 'n': /* null */
-                return json_null();
+static void set_error(scanner_t *s, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    jsonp_error_vset(s->error, s->line, s->column, fmt, ap);
+    va_end(ap);
+}
 
-            case 'b': /* boolean */
-                obj = va_arg(*ap, int) ?
-                    json_true() : json_false();
-                return obj;
+static json_t *pack(scanner_t *s, va_list *ap);
 
-            case 'i': /* integer */
-                return json_integer(va_arg(*ap, int));
+static json_t *pack_object(scanner_t *s, va_list *ap)
+{
+    json_t *object = json_object();
+    next_token(s);
 
-            case 'f': /* double-precision float */
-                return json_real(va_arg(*ap, double));
+    while(s->token != '}') {
+        const char *key;
+        json_t *value;
 
-            case 'O': /* a json_t object; increments refcount */
-                obj = va_arg(*ap, json_t *);
-                json_incref(obj);
-                return obj;
+        if(!s->token) {
+            set_error(s, "Unexpected end of format string");
+            goto error;
+        }
 
-            case 'o': /* a json_t object; doesn't increment refcount */
-                obj = va_arg(*ap, json_t *);
-                return obj;
+        if(s->token != 's') {
+            set_error(s, "Expected format 's', got '%c'\n", *s->fmt);
+            goto error;
+        }
 
-            default: /* Whoops! */
-                jsonp_error_set(error, 1, 1,
-                        "Didn't understand format character '%c'",
-                        *tok);
-                return NULL;
+        key = va_arg(*ap, const char *);
+        if(!key) {
+            set_error(s, "NULL object key");
+            goto error;
         }
-    }
 
-    /* Move past container opening token */
-    tok++;
-    column++;
+        next_token(s);
 
-    while(tok-fmt < size) {
-        switch(*tok) {
-            case '\n':
-                line++;
-                column = 0;
-                break;
+        value = pack(s, ap);
+        if(!value)
+            goto error;
 
-            case ' ': /* Whitespace */
-                break;
+        if(json_object_set_new(object, key, value)) {
+            set_error(s, "Unable to add key \"%s\"", key);
+            goto error;
+        }
 
-            case ',': /* Element spacer */
-                if(key)
-                {
-                    jsonp_error_set(error, line, column,
-                              "Expected KEY, got COMMA!");
-                    json_decref(root);
-                    return NULL;
-                }
-                break;
+        next_token(s);
+    }
 
-            case ':': /* Key/value separator */
-                if(!key)
-                {
-                    jsonp_error_set(error, line, column,
-                              "Got key/value separator without "
-                              "a key preceding it!");
-                    json_decref(root);
-                    return NULL;
-                }
+    return object;
 
-                if(!json_is_object(root))
-                {
-                    jsonp_error_set(error, line, column,
-                              "Got a key/value separator "
-                              "(':') outside an object!");
-                    json_decref(root);
-                    return NULL;
-                }
+error:
+    json_decref(object);
+    return NULL;
+}
 
-                break;
+static json_t *pack_array(scanner_t *s, va_list *ap)
+{
+    json_t *array = json_array();
+    next_token(s);
 
-            case ']': /* Close array or object */
-            case '}':
+    while(s->token != ']') {
+        json_t *value;
 
-                if(tok-fmt + (ssize_t)strspn(tok+1, " ") != size-1)
-                {
-                    jsonp_error_set(error, line, column,
-                              "Unexpected close-bracket '%c'", *tok);
-                    json_decref(root);
-                    return NULL;
-                }
-
-                if((*tok == ']' && !json_is_array(root)) ||
-                   (*tok == '}' && !json_is_object(root)))
-                {
-                    jsonp_error_set(error, line, column,
-                              "Stray close-array '%c' character", *tok);
-                    json_decref(root);
-                    return NULL;
-                }
-                return root;
+        if(!s->token) {
+            set_error(s, "Unexpected end of format string");
+            goto error;
+        }
 
-            case '[':
-            case '{':
+        value = pack(s, ap);
+        if(!value)
+            goto error;
 
-                /* Shortcut so we don't mess up the column count in error
-                 * messages */
-                if(json_is_object(root) && !key)
-                    goto common;
+        if(json_array_append_new(array, value)) {
+            set_error(s, "Unable to append to array");
+            goto error;
+        }
 
-                /* Find corresponding close bracket */
-                etok = tok+1;
-                etok_depth = 1;
-                while(etok_depth) {
+        next_token(s);
+    }
+    return array;
 
-                    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;
-                    }
+error:
+    json_decref(array);
+    return NULL;
+}
 
-                    if(*tok == *etok)
-                        etok_depth++;
-                    else if(*tok == '[' && *etok == ']') {
-                        etok_depth--;
-                        break;
-                    } else if(*tok == '{' && *etok == '}') {
-                        etok_depth--;
-                        break;
-                    }
+static json_t *pack(scanner_t *s, va_list *ap)
+{
+    switch(s->token) {
+        case '{':
+            return pack_object(s, ap);
 
-                    etok++;
-                }
+        case '[':
+            return pack_array(s, ap);
 
-                /* 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 */
+        {
+            const char *str = va_arg(*ap, const char *);
+            if(!str)
+            {
+                set_error(s, "NULL string");
+                return NULL;
+            }
+            return json_string(str);
+        }
 
-            case 's':
-                /* Handle strings specially, since they're used for both keys
-                 * and values */
-                s = va_arg(*ap, char *);
+        case 'n': /* null */
+            return json_null();
 
-                if(!s)
-                {
-                    jsonp_error_set(error, line, column,
-                              "Refusing to handle a NULL string");
-                    json_decref(root);
-                    return NULL;
-                }
+        case 'b': /* boolean */
+            return va_arg(*ap, int) ? json_true() : json_false();
 
-                if(json_is_object(root) && !key)
-                {
-                    /* It's a key */
-                    key = s;
-                    break;
-                }
+        case 'i': /* integer */
+            return json_integer(va_arg(*ap, int));
 
-                obj = json_string(s);
-                goto common;
+        case 'f': /* double-precision float */
+            return json_real(va_arg(*ap, double));
 
-        default:
-                obj = json_vnpack(error, 1, tok, ap);
-                if(!obj) {
-                    json_decref(root);
-                    return NULL;
-                }
+        case 'O': /* a json_t object; increments refcount */
+            return json_incref(va_arg(*ap, json_t *));
 
-common:
-                /* Add to container */
-                if(json_is_object(root)) {
-                    if(!key)
-                    {
-                        jsonp_error_set(error, line, column,
-                              "Expected key, got identifier '%c'!", *tok);
-                        json_decref(root);
-                        return NULL;
-                    }
+        case 'o': /* a json_t object; doesn't increment refcount */
+            return va_arg(*ap, json_t *);
 
-                    json_object_set_new(root, key, obj);
-                    key = NULL;
-                }
-                else
-                {
-                    json_array_append_new(root, obj);
-                }
-                break;
-        }
-        tok++;
-        column++;
+        default: /* Whoops! */
+            set_error(s, "Unrecognized format character '%c'", s->token);
+            return NULL;
     }
-
-    /* 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)
@@ -555,8 +457,9 @@ static int json_vnunpack(json_t *root, json_error_t *error, ssize_t size, const
 
 json_t *json_pack(json_error_t *error, const char *fmt, ...)
 {
+    scanner_t s;
+    json_t *value;
     va_list ap;
-    json_t *obj;
 
     jsonp_error_init(error, "");
 
@@ -565,11 +468,24 @@ json_t *json_pack(json_error_t *error, const char *fmt, ...)
         return NULL;
     }
 
+    s.error = error;
+    s.fmt = fmt;
+    s.line = 1;
+    s.column = 0;
+
+    next_token(&s);
+
     va_start(ap, fmt);
-    obj = json_vpack(error, fmt, &ap);
+    value = pack(&s, &ap);
     va_end(ap);
 
-    return obj;
+    next_token(&s);
+    if(s.token) {
+        set_error(&s, "Garbage after format string");
+        return NULL;
+    }
+
+    return value;
 }
 
 int json_unpack(json_t *root, json_error_t *error, const char *fmt, ...)
diff --git a/test/suites/api/test_pack.c b/test/suites/api/test_pack.c
index ab68d16..7ca58ed 100644
--- a/test/suites/api/test_pack.c
+++ b/test/suites/api/test_pack.c
@@ -191,7 +191,7 @@ int main()
 
     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)
+    if(error.line != 1 || error.column != 19)
         fail("json_pack didn't get the error coordinates right!");
 
     if(json_pack(&error, "[[[[[   [[[[[  [[[[ }]]]] ]]]] ]]]]]"))
-- 
2.1.4