[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] qjson: replace QString in JSONLexer with GS
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] qjson: replace QString in JSONLexer with GString |
Date: |
Fri, 20 Nov 2015 21:16:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 20/11/2015 19:23, Eric Blake wrote:
> On 11/20/2015 02:04 AM, Paolo Bonzini wrote:
>> JSONLexer only needs a simple resizable buffer. json-streamer.c
>> can allocate memory for each token instead of relying on reference
>> counting of QStrings.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>> include/qapi/qmp/json-lexer.h | 7 +++----
>> include/qapi/qmp/json-streamer.h | 1 +
>> qobject/json-lexer.c | 22 ++++++++--------------
>> qobject/json-streamer.c | 10 +++++-----
>> 4 files changed, 17 insertions(+), 23 deletions(-)
>>
>
>> --- a/include/qapi/qmp/json-streamer.h
>> +++ b/include/qapi/qmp/json-streamer.h
>> @@ -14,6 +14,7 @@
>> #ifndef QEMU_JSON_STREAMER_H
>> #define QEMU_JSON_STREAMER_H
>>
>> +#include <stdint.h>
>> #include "qapi/qmp/qlist.h"
>> #include "qapi/qmp/json-lexer.h"
>>
>
> Spurious hunk? If not, should mention in the commit message why this is
> needed.
Does "because otherwise it doesn't compile" count? :)
It was relying on json-lexer.h indirectly including stdint.h. I don't
think it ought to be mentioned in the commit message.
Paolo
>> +++ b/qobject/json-streamer.c
>> @@ -12,6 +12,7 @@
>> */
>>
>> #include "qapi/qmp/qlist.h"
>> +#include "qapi/qmp/qstring.h"
>> #include "qapi/qmp/qint.h"
>> #include "qapi/qmp/qdict.h"
>> #include "qemu-common.h"
>> @@ -21,13 +22,13 @@
>> #define MAX_TOKEN_SIZE (64ULL << 20)
>> #define MAX_NESTING (1ULL << 10)
>>
>> -static void json_message_process_token(JSONLexer *lexer, QString *token,
>> JSONTokenType type, int x, int y)
>> +static void json_message_process_token(JSONLexer *lexer, GString *input,
>> JSONTokenType type, int x, int y)
>
> Worth wrapping the long line while touching it?
>
> Otherwise looks like a straightforward conversion. If the commit
> message is fixed, you can add
>
> Reviewed-by: Eric Blake <address@hidden>
>