Message ID | 20200402121313.GA5563@simran-Inspiron-5558 (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | qobject: json-streamer: Remove double test | expand |
On 4/2/20 7:13 AM, Simran Singhal wrote: > Remove the duplicate test "parser->bracket_count >= 0". > > Signed-off-by: Simran Singhal <singhalsimran0@gmail.com> > --- > 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.
On 4/2/20 8:12 AM, Eric Blake wrote: > On 4/2/20 7:13 AM, Simran Singhal wrote: >> Remove the duplicate test "parser->bracket_count >= 0". >> >> Signed-off-by: Simran Singhal <singhalsimran0@gmail.com> >> --- >> qobject/json-streamer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> (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. Effect of the bug: Note that we can diagnose when we have unbalanced ] with no matching [ while inside {}: $ qemu-kvm --nodefaults --nographic --qmp stdio {"QMP": {"version": {"qemu": {"micro": 91, "minor": 2, "major": 4}, "package": "v5.0.0-rc1-1-gf6ce4a439a08"}, "capabilities": ["oob"]}} {] {"error": {"class": "GenericError", "desc": "JSON parse error, expecting value"}} but that we fail to diagnose unbalanced } with no matching { while inside []: [}
On Thu, Apr 2, 2020 at 6:42 PM Eric Blake <eblake@redhat.com> wrote: > On 4/2/20 7:13 AM, Simran Singhal wrote: > > Remove the duplicate test "parser->bracket_count >= 0". > > > > Signed-off-by: Simran Singhal <singhalsimran0@gmail.com> > > --- > > 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. > Hello Eric, Thanks for the explanation. I'll send v2 with the required changes. -- Simran > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > >
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); if ((parser->brace_count > 0 || parser->bracket_count > 0) - && parser->bracket_count >= 0 && parser->bracket_count >= 0) { + && parser->bracket_count >= 0) { return; }
Remove the duplicate test "parser->bracket_count >= 0". Signed-off-by: Simran Singhal <singhalsimran0@gmail.com> --- qobject/json-streamer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)