Re: [Qemu-devel] [PATCH v2 2/4] qjson: do not save/restore contexts

[Laszlo Ersek <lersek@redhat.com>]

On 11/23/15 18:44, 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. ಠ_ಠ

Is it accepted practice to put UTF-8 in commit messages? (Or, actually,
anywhere in patches, except maybe the notes section?)

I'd recommend o_O.

Thanks
Laszlo

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