Re: [PATCH] qobject: json-streamer: Remove double test

[Eric Blake]

On 4/2/20 7:13 AM, Simran Singhal wrote:
> Remove the duplicate test "parser->bracket_count >= 0".
>
> Signed-off-by: Simran Singhal <address@hidden>
> ---
>   qobject/json-streamer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> index 47dd7ea576..ef48185283 100644
> --- a/qobject/json-streamer.c
> +++ b/qobject/json-streamer.c
> @@ -85,7 +85,7 @@ void json_message_process_token(JSONLexer *lexer, GString 
> *input,
>       g_queue_push_tail(&parser->tokens, token);
>   

Adding some context:

>       if ((parser->brace_count > 0 || parser->bracket_count > 0)
> -        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
> +        && parser->bracket_count >= 0) {
>           return;
>       }
>   
>     json = json_parser_parse(parser->tokens, parser->ap, &err);
>     parser->tokens = NULL;
>
> out_emit:

This code was rewritten in commit 8d3265b3.  Prior to that, it read:


    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;
    }

    return;

out_emit:

Obviously, the goal of the rewrite was to convert:

if (cond) {
  do stuff
} else {
  return
}
more stuff

into the more legible

if (!cond) {
  return
}
do stuff
more stuff

Let's re-read my original review:

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03017.html

> 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

What I missed was the typo: we checked bracket >= 0 twice, instead of 
the intended brace >= 0 && bracket >= 0.  This needs a v2.

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