* [PATCH] qobject: json-streamer: Remove double test
@ 2020-04-02 12:13 Simran Singhal
2020-04-02 13:12 ` Eric Blake
0 siblings, 1 reply; 4+ messages in thread
From: Simran Singhal @ 2020-04-02 12:13 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Stefan Hajnoczi, Julia Suvorova
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);
if ((parser->brace_count > 0 || parser->bracket_count > 0)
- && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
+ && parser->bracket_count >= 0) {
return;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] qobject: json-streamer: Remove double test
2020-04-02 12:13 [PATCH] qobject: json-streamer: Remove double test Simran Singhal
@ 2020-04-02 13:12 ` Eric Blake
2020-04-02 13:19 ` Eric Blake
2020-04-02 16:41 ` Simran Singhal
0 siblings, 2 replies; 4+ messages in thread
From: Eric Blake @ 2020-04-02 13:12 UTC (permalink / raw)
To: Simran Singhal, Markus Armbruster, qemu-devel
Cc: Stefan Hajnoczi, Julia Suvorova
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qobject: json-streamer: Remove double test
2020-04-02 13:12 ` Eric Blake
@ 2020-04-02 13:19 ` Eric Blake
2020-04-02 16:41 ` Simran Singhal
1 sibling, 0 replies; 4+ messages in thread
From: Eric Blake @ 2020-04-02 13:19 UTC (permalink / raw)
To: Simran Singhal, Markus Armbruster, qemu-devel
Cc: Stefan Hajnoczi, Julia Suvorova
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 []:
[}
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] qobject: json-streamer: Remove double test
2020-04-02 13:12 ` Eric Blake
2020-04-02 13:19 ` Eric Blake
@ 2020-04-02 16:41 ` Simran Singhal
1 sibling, 0 replies; 4+ messages in thread
From: Simran Singhal @ 2020-04-02 16:41 UTC (permalink / raw)
To: Eric Blake; +Cc: Stefan Hajnoczi, Julia Suvorova, Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 4378 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-02 16:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 12:13 [PATCH] qobject: json-streamer: Remove double test Simran Singhal
2020-04-02 13:12 ` Eric Blake
2020-04-02 13:19 ` Eric Blake
2020-04-02 16:41 ` Simran Singhal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.