Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/json-streamer.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index 810aae521f..954bf9d468 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -99,16 +99,13 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
 
     g_queue_push_tail(parser->tokens, token);
 
-    if (parser->brace_count < 0 ||
-        parser->bracket_count < 0 ||
-        (parser->brace_count == 0 &&
-         parser->bracket_count == 0)) {
-        json = json_parser_parse(parser->tokens, parser->ap, &err);
-        parser->tokens = NULL;
-        goto out_emit;
+    if ((parser->brace_count > 0 || parser->bracket_count > 0)
+        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
+        return;
     }
 
-    return;
+    json = json_parser_parse(parser->tokens, parser->ap, &err);
+    parser->tokens = NULL;
 
 out_emit:
     parser->brace_count = 0;
-- 
2.17.1

On 08/08/2018 07:03 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qobject/json-streamer.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> index 810aae521f..954bf9d468 100644
> --- a/qobject/json-streamer.c
> +++ b/qobject/json-streamer.c
> @@ -99,16 +99,13 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
>   
>       g_queue_push_tail(parser->tokens, token);
>   
> -    if (parser->brace_count < 0 ||
> -        parser->bracket_count < 0 ||

Old: if we are unbalanced (more right tokens read than left)...

> -        (parser->brace_count == 0 &&
> -         parser->bracket_count == 0)) {

...or if we uniformly ended nesting,...

> -        json = json_parser_parse(parser->tokens, parser->ap, &err);

...then parse (to either diagnose the unbalance, or to see if the 
balanced construct is valid), with weird flow control that skips over an 
early return.

Or put another way, if we invert the condition, we find the cases where 
we want an early return instead of parsing (and can thus use that to get 
rid of an unsightly goto over a single early return).

Applying deMorgan's rules:

!(brace < 0 || bracket < 0 || (brace == 0 && bracket == 0))
!(brace < 0) && !(bracket < 0) && !(brace == 0 && bracket == 0)
brace >= 0 && bracket >= 0 && (!(brace == 0) || !(bracket == 0))
brace >= 0 && bracket >= 0 && (brace != 0 || bracket != 0)

But based on what we learned in the first two conjunctions, we can 
rewrite the third:

brace >= 0 && bracket >= 0 && (brace > 0 || bracket > 0)

and then commute the logic:

(brace > 0 || bracket > 0) && brace >= 0 && bracket >= 0

> -        parser->tokens = NULL;
> -        goto out_emit;
> +    if ((parser->brace_count > 0 || parser->bracket_count > 0)
> +        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {

So the new condition is correct, and reads as:

If either struct is still awaiting closure, and both structs have not 
gone unbalanced, then early exit.

It was not intuitive, but stepping through the logic shows it is identical.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 08/08/2018 07:03 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qobject/json-streamer.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
>> index 810aae521f..954bf9d468 100644
>> --- a/qobject/json-streamer.c
>> +++ b/qobject/json-streamer.c
>> @@ -99,16 +99,13 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
>>         g_queue_push_tail(parser->tokens, token);
>>   -    if (parser->brace_count < 0 ||
>> -        parser->bracket_count < 0 ||
>
> Old: if we are unbalanced (more right tokens read than left)...
>
>> -        (parser->brace_count == 0 &&
>> -         parser->bracket_count == 0)) {
>
> ...or if we uniformly ended nesting,...
>
>> -        json = json_parser_parse(parser->tokens, parser->ap, &err);
>
> ...then parse (to either diagnose the unbalance, or to see if the
> balanced construct is valid), with weird flow control that skips over
> an early return.
>
> Or put another way, if we invert the condition, we find the cases
> where we want an early return instead of parsing (and can thus use
> that to get rid of an unsightly goto over a single early return).
>
> Applying deMorgan's rules:
>
> !(brace < 0 || bracket < 0 || (brace == 0 && bracket == 0))
> !(brace < 0) && !(bracket < 0) && !(brace == 0 && bracket == 0)
> brace >= 0 && bracket >= 0 && (!(brace == 0) || !(bracket == 0))
> brace >= 0 && bracket >= 0 && (brace != 0 || bracket != 0)
>
> But based on what we learned in the first two conjunctions, we can
> rewrite the third:
>
> brace >= 0 && bracket >= 0 && (brace > 0 || bracket > 0)
>
> and then commute the logic:
>
> (brace > 0 || bracket > 0) && brace >= 0 && bracket >= 0
>
>> -        parser->tokens = NULL;
>> -        goto out_emit;
>> +    if ((parser->brace_count > 0 || parser->bracket_count > 0)
>> +        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
>
> So the new condition is correct, and reads as:
>
> If either struct is still awaiting closure, and both structs have not
> gone unbalanced, then early exit.
>
> It was not intuitive, but stepping through the logic shows it is identical.

My first version had a "simpler" condition there.  My test cases proved
it wrong %-}

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!