diff mbox

[2/4] qjson: do not save/restore contexts

Message ID 1448010269-21694-3-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Nov. 20, 2015, 9:04 a.m. UTC 
JSON is LL(1) and our parser indeed needs only 1 token lookahead.
Saving the parser context is mostly unnecessary; we can replace it
with peeking at the next token, or remove it altogether when the
restore only happens on errors.  The token list is destroyed anyway
on errors.

The only interesting thing is that parse_keyword always eats
a TOKEN_KEYWORD, even if it is invalid, so it must come last in
parse_value (otherwise, NULL is returned, parse_literal is invoked
and it tries to peek beyond end of input).  This is caught by
/errors/unterminated/literal, which actually checks for an unterminated
keyword. ಠ_ಠ

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qobject/json-parser.c | 59 ++++++++++++++++++---------------------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

Comments

Eric Blake Nov. 20, 2015, 6:28 p.m. UTC | #1 
On 11/20/2015 02:04 AM, Paolo Bonzini wrote:
> JSON is LL(1) and our parser indeed needs only 1 token lookahead.
> Saving the parser context is mostly unnecessary; we can replace it
> with peeking at the next token, or remove it altogether when the
> restore only happens on errors.  The token list is destroyed anyway
> on errors.
> 
> The only interesting thing is that parse_keyword always eats
> a TOKEN_KEYWORD, even if it is invalid, so it must come last in
> parse_value (otherwise, NULL is returned, parse_literal is invoked
> and it tries to peek beyond end of input).  This is caught by
> /errors/unterminated/literal, which actually checks for an unterminated
> keyword. ಠ_ಠ
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qobject/json-parser.c | 59 ++++++++++++++++++---------------------------------
>  1 file changed, 21 insertions(+), 38 deletions(-)
> 

> @@ -578,17 +556,21 @@ out:
>  static QObject *parse_escape(JSONParserContext *ctxt, va_list *ap)
>  {
>      QObject *token = NULL, *obj;
> -    JSONParserContext saved_ctxt = parser_context_save(ctxt);
>  
>      if (ap == NULL) {
>          goto out;
>      }
>  
> -    token = parser_context_pop_token(ctxt);
> +    token = parser_context_peek_token(ctxt);
>      if (token == NULL) {
>          goto out;
>      }
>  
> +    if (token_get_type(token) != JSON_ESCAPE) {
> +        goto out;
> +    }

Could merge these two conditionals.

Otherwise, makes sense to me, and a lot less complicated.

Reviewed-by: Eric Blake <eblake@redhat.com>
Paolo Bonzini Nov. 20, 2015, 8:17 p.m. UTC | #2 
On 20/11/2015 19:28, Eric Blake wrote:
>> > +    token = parser_context_peek_token(ctxt);
>> >      if (token == NULL) {
>> >          goto out;
>> >      }
>> >  
>> > +    if (token_get_type(token) != JSON_ESCAPE) {
>> > +        goto out;
>> > +    }
> Could merge these two conditionals.

Following the style of the rest of the file, for example:

    if (token == NULL) {
        goto out;
    }

    if (!token_is_operator(token, '{')) {
        goto out;
    }

Paolo
diff mbox

Patch

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index ac991ba..7a287ea 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -296,23 +296,6 @@  static QObject *parser_context_peek_token(JSONParserContext *ctxt)
     return token;
 }
 
-static JSONParserContext parser_context_save(JSONParserContext *ctxt)
-{
-    JSONParserContext saved_ctxt = {0};
-    saved_ctxt.tokens.pos = ctxt->tokens.pos;
-    saved_ctxt.tokens.count = ctxt->tokens.count;
-    saved_ctxt.tokens.buf = ctxt->tokens.buf;
-    return saved_ctxt;
-}
-
-static void parser_context_restore(JSONParserContext *ctxt,
-                                   JSONParserContext saved_ctxt)
-{
-    ctxt->tokens.pos = saved_ctxt.tokens.pos;
-    ctxt->tokens.count = saved_ctxt.tokens.count;
-    ctxt->tokens.buf = saved_ctxt.tokens.buf;
-}
-
 static void tokens_append_from_iter(QObject *obj, void *opaque)
 {
     JSONParserContext *ctxt = opaque;
@@ -364,7 +347,6 @@  static void parser_context_free(JSONParserContext *ctxt)
 static int parse_pair(JSONParserContext *ctxt, QDict *dict, va_list *ap)
 {
     QObject *key = NULL, *token = NULL, *value, *peek;
-    JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
     peek = parser_context_peek_token(ctxt);
     if (peek == NULL) {
@@ -402,7 +384,6 @@  static int parse_pair(JSONParserContext *ctxt, QDict *dict, va_list *ap)
     return 0;
 
 out:
-    parser_context_restore(ctxt, saved_ctxt);
     qobject_decref(key);
 
     return -1;
@@ -412,9 +393,8 @@  static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
 {
     QDict *dict = NULL;
     QObject *token, *peek;
-    JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
-    token = parser_context_pop_token(ctxt);
+    token = parser_context_peek_token(ctxt);
     if (token == NULL) {
         goto out;
     }
@@ -425,6 +405,7 @@  static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
 
     dict = qdict_new();
 
+    parser_context_pop_token(ctxt);
     peek = parser_context_peek_token(ctxt);
     if (peek == NULL) {
         parse_error(ctxt, NULL, "premature EOI");
@@ -465,7 +446,6 @@  static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
     return QOBJECT(dict);
 
 out:
-    parser_context_restore(ctxt, saved_ctxt);
     QDECREF(dict);
     return NULL;
 }
@@ -474,9 +454,8 @@  static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
 {
     QList *list = NULL;
     QObject *token, *peek;
-    JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
-    token = parser_context_pop_token(ctxt);
+    token = parser_context_peek_token(ctxt);
     if (token == NULL) {
         goto out;
     }
@@ -487,6 +466,7 @@  static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
 
     list = qlist_new();
 
+    parser_context_pop_token(ctxt);
     peek = parser_context_peek_token(ctxt);
     if (peek == NULL) {
         parse_error(ctxt, NULL, "premature EOI");
@@ -537,7 +517,6 @@  static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
     return QOBJECT(list);
 
 out:
-    parser_context_restore(ctxt, saved_ctxt);
     QDECREF(list);
     return NULL;
 }
@@ -545,9 +524,8 @@  out:
 static QObject *parse_keyword(JSONParserContext *ctxt)
 {
     QObject *token, *ret;
-    JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
-    token = parser_context_pop_token(ctxt);
+    token = parser_context_peek_token(ctxt);
     if (token == NULL) {
         goto out;
     }
@@ -556,6 +534,7 @@  static QObject *parse_keyword(JSONParserContext *ctxt)
         goto out;
     }
 
+    parser_context_pop_token(ctxt);
     if (token_is_keyword(token, "true")) {
         ret = QOBJECT(qbool_from_bool(true));
     } else if (token_is_keyword(token, "false")) {
@@ -570,7 +549,6 @@  static QObject *parse_keyword(JSONParserContext *ctxt)
     return ret;
 
 out: 
-    parser_context_restore(ctxt, saved_ctxt);
 
     return NULL;
 }
@@ -578,17 +556,21 @@  out:
 static QObject *parse_escape(JSONParserContext *ctxt, va_list *ap)
 {
     QObject *token = NULL, *obj;
-    JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
     if (ap == NULL) {
         goto out;
     }
 
-    token = parser_context_pop_token(ctxt);
+    token = parser_context_peek_token(ctxt);
     if (token == NULL) {
         goto out;
     }
 
+    if (token_get_type(token) != JSON_ESCAPE) {
+        goto out;
+    }
+
+    parser_context_pop_token(ctxt);
     if (token_is_escape(token, "%p")) {
         obj = va_arg(*ap, QObject *);
     } else if (token_is_escape(token, "%i")) {
@@ -611,7 +593,6 @@  static QObject *parse_escape(JSONParserContext *ctxt, va_list *ap)
     return obj;
 
 out:
-    parser_context_restore(ctxt, saved_ctxt);
 
     return NULL;
 }
@@ -619,15 +600,15 @@  out:
 static QObject *parse_literal(JSONParserContext *ctxt)
 {
     QObject *token, *obj;
-    JSONParserContext saved_ctxt = parser_context_save(ctxt);
 
-    token = parser_context_pop_token(ctxt);
+    token = parser_context_peek_token(ctxt);
     if (token == NULL) {
         goto out;
     }
 
     switch (token_get_type(token)) {
     case JSON_STRING:
+        parser_context_pop_token(ctxt);
         obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
         break;
     case JSON_INTEGER: {
@@ -645,15 +626,18 @@  static QObject *parse_literal(JSONParserContext *ctxt)
          */
         int64_t value;
 
+        parser_context_pop_token(ctxt);
         errno = 0; /* strtoll doesn't set errno on success */
         value = strtoll(token_get_value(token), NULL, 10);
         if (errno != ERANGE) {
             obj = QOBJECT(qint_from_int(value));
             break;
         }
-        /* fall through to JSON_FLOAT */
+        goto parse_float;
     }
     case JSON_FLOAT:
+        parser_context_pop_token(ctxt);
+    parse_float:
         /* FIXME dependent on locale */
         obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), NULL)));
         break;
@@ -664,7 +648,6 @@  static QObject *parse_literal(JSONParserContext *ctxt)
     return obj;
 
 out:
-    parser_context_restore(ctxt, saved_ctxt);
 
     return NULL;
 }
@@ -681,11 +664,11 @@  static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
         obj = parse_escape(ctxt, ap);
     }
     if (obj == NULL) {
-        obj = parse_keyword(ctxt);
-    } 
-    if (obj == NULL) {
         obj = parse_literal(ctxt);
     }
+    if (obj == NULL) {
+        obj = parse_keyword(ctxt);
+    }
 
     return obj;
 }