Re: [Qemu-devel] [PATCH 1/4] qjson: replace QString in JSONLexer with GString

[Paolo Bonzini <pbonzini@redhat.com>]

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 <pbonzini@redhat.com>
>> ---
>>  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 <eblake@redhat.com>
>