* [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets @ 2017-04-06 15:41 Dr. David Alan Gilbert (git) 2017-04-06 15:49 ` Eric Blake 2017-04-12 9:04 ` [Qemu-devel] [PATCH] " Markus Armbruster 0 siblings, 2 replies; 6+ messages in thread From: Dr. David Alan Gilbert (git) @ 2017-04-06 15:41 UTC (permalink / raw) To: qemu-devel, armbru, mreitz From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Gcc 7 (on Fedora 26) spotted odd use of integers instead of a boolean; it's got a point. Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> --- tests/check-qdict.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/check-qdict.c b/tests/check-qdict.c index 81162ee572..6f3fbcf9c1 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -559,7 +559,7 @@ static void qdict_join_test(void) g_assert(qdict_size(dict1) == 2); g_assert(qdict_size(dict2) == !overwrite); - g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42); + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); g_assert(qdict_get_int(dict1, "bar") == 23); if (!overwrite) { -- 2.12.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets 2017-04-06 15:41 [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets Dr. David Alan Gilbert (git) @ 2017-04-06 15:49 ` Eric Blake 2017-04-06 15:52 ` Dr. David Alan Gilbert 2017-04-06 15:55 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake 2017-04-12 9:04 ` [Qemu-devel] [PATCH] " Markus Armbruster 1 sibling, 2 replies; 6+ messages in thread From: Eric Blake @ 2017-04-06 15:49 UTC (permalink / raw) To: Dr. David Alan Gilbert (git), qemu-devel, armbru, mreitz [-- Attachment #1: Type: text/plain, Size: 1103 bytes --] On 04/06/2017 10:41 AM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Gcc 7 (on Fedora 26) spotted odd use of integers instead of a > boolean; it's got a point. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tests/check-qdict.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/check-qdict.c b/tests/check-qdict.c > index 81162ee572..6f3fbcf9c1 100644 > --- a/tests/check-qdict.c > +++ b/tests/check-qdict.c > @@ -559,7 +559,7 @@ static void qdict_join_test(void) > g_assert(qdict_size(dict1) == 2); > g_assert(qdict_size(dict2) == !overwrite); > > - g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42); > + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); How is the test passing pre-patch, and why does it not change the test post-patch? Does that mean that overwrite is not doing what we expected? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets 2017-04-06 15:49 ` Eric Blake @ 2017-04-06 15:52 ` Dr. David Alan Gilbert 2017-04-06 15:55 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake 1 sibling, 0 replies; 6+ messages in thread From: Dr. David Alan Gilbert @ 2017-04-06 15:52 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel, armbru, mreitz * Eric Blake (eblake@redhat.com) wrote: > On 04/06/2017 10:41 AM, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Gcc 7 (on Fedora 26) spotted odd use of integers instead of a > > boolean; it's got a point. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > tests/check-qdict.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/check-qdict.c b/tests/check-qdict.c > > index 81162ee572..6f3fbcf9c1 100644 > > --- a/tests/check-qdict.c > > +++ b/tests/check-qdict.c > > @@ -559,7 +559,7 @@ static void qdict_join_test(void) > > g_assert(qdict_size(dict1) == 2); > > g_assert(qdict_size(dict2) == !overwrite); > > > > - g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42); > > + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); > > How is the test passing pre-patch, and why does it not change the test > post-patch? Does that mean that overwrite is not doing what we expected? Pre-patch it passes because either 84 or 42 are valid 'true' values into g_assert. Post patch it ends up as either: qdict_get_int(dict1, "foo") == 42 or qdict_get_int(dict1, "foo") == 84 which is what's intended. Dave > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] tests/check-qdict: Fix missing brackets 2017-04-06 15:49 ` Eric Blake 2017-04-06 15:52 ` Dr. David Alan Gilbert @ 2017-04-06 15:55 ` Eric Blake 2017-04-07 21:27 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 6+ messages in thread From: Eric Blake @ 2017-04-06 15:55 UTC (permalink / raw) To: Dr. David Alan Gilbert (git), qemu-devel, armbru, mreitz [-- Attachment #1: Type: text/plain, Size: 1836 bytes --] On 04/06/2017 10:49 AM, Eric Blake wrote: > On 04/06/2017 10:41 AM, Dr. David Alan Gilbert (git) wrote: >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> Gcc 7 (on Fedora 26) spotted odd use of integers instead of a >> boolean; it's got a point. >> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> --- >> tests/check-qdict.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tests/check-qdict.c b/tests/check-qdict.c >> index 81162ee572..6f3fbcf9c1 100644 >> --- a/tests/check-qdict.c >> +++ b/tests/check-qdict.c >> @@ -559,7 +559,7 @@ static void qdict_join_test(void) >> g_assert(qdict_size(dict1) == 2); >> g_assert(qdict_size(dict2) == !overwrite); >> >> - g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42); >> + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); > > How is the test passing pre-patch, and why does it not change the test > post-patch? Does that mean that overwrite is not doing what we expected? Replying to myself: Pre-patch, it was reached twice (once for overwrite=false, once for overwrite=true), as: (42 == false) ? 84 : 42 (84 == true) ? 84 : 42 which simplifies to 42 on both iterations, and g_assert(42) succeeds. In fact, this is a tautology - no matter WHAT value we encounter, the assert succeeds, so we are not actually testing that the value matters. Post-patch, it becomes: 42 == (false ? 84 : 42) 84 == (true ? 84 : 42) which is then asserting that we actually have the value we expect. Reviewed-by: Eric Blake <eblake@redhat.com> Safe for 2.9, but late enough that it also doesn't matter if it slips until 2.10. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] tests/check-qdict: Fix missing brackets 2017-04-06 15:55 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake @ 2017-04-07 21:27 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2017-04-07 21:27 UTC (permalink / raw) To: Eric Blake, Dr. David Alan Gilbert (git), qemu-devel, armbru, mreitz On 04/06/2017 12:55 PM, Eric Blake wrote: > On 04/06/2017 10:49 AM, Eric Blake wrote: >> On 04/06/2017 10:41 AM, Dr. David Alan Gilbert (git) wrote: >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>> >>> Gcc 7 (on Fedora 26) spotted odd use of integers instead of a >>> boolean; it's got a point. >>> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> --- >>> tests/check-qdict.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tests/check-qdict.c b/tests/check-qdict.c >>> index 81162ee572..6f3fbcf9c1 100644 >>> --- a/tests/check-qdict.c >>> +++ b/tests/check-qdict.c >>> @@ -559,7 +559,7 @@ static void qdict_join_test(void) >>> g_assert(qdict_size(dict1) == 2); >>> g_assert(qdict_size(dict2) == !overwrite); >>> >>> - g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42); >>> + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); >> >> How is the test passing pre-patch, and why does it not change the test >> post-patch? Does that mean that overwrite is not doing what we expected? > > Replying to myself: > > Pre-patch, it was reached twice (once for overwrite=false, once for > overwrite=true), as: > > (42 == false) ? 84 : 42 > (84 == true) ? 84 : 42 > > which simplifies to 42 on both iterations, and g_assert(42) succeeds. > In fact, this is a tautology - no matter WHAT value we encounter, the > assert succeeds, so we are not actually testing that the value matters. > > Post-patch, it becomes: > > 42 == (false ? 84 : 42) > 84 == (true ? 84 : 42) > > which is then asserting that we actually have the value we expect. > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Safe for 2.9, but late enough that it also doesn't matter if it slips > until 2.10. > another case which demonstrate human _optimized_ C is more bug prone than verbose basic code... Most cpp generates the same asm code with the following C: if (overwrite) { g_assert(qdict_get_int(dict1, "foo") == 84); } else { g_assert(qdict_get_int(dict1, "foo") == 42); } Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets 2017-04-06 15:41 [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets Dr. David Alan Gilbert (git) 2017-04-06 15:49 ` Eric Blake @ 2017-04-12 9:04 ` Markus Armbruster 1 sibling, 0 replies; 6+ messages in thread From: Markus Armbruster @ 2017-04-12 9:04 UTC (permalink / raw) To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, mreitz "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Gcc 7 (on Fedora 26) spotted odd use of integers instead of a > boolean; it's got a point. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tests/check-qdict.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/check-qdict.c b/tests/check-qdict.c > index 81162ee572..6f3fbcf9c1 100644 > --- a/tests/check-qdict.c > +++ b/tests/check-qdict.c > @@ -559,7 +559,7 @@ static void qdict_join_test(void) > g_assert(qdict_size(dict1) == 2); > g_assert(qdict_size(dict2) == !overwrite); > > - g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42); > + g_assert(qdict_get_int(dict1, "foo") == (overwrite ? 84 : 42)); > g_assert(qdict_get_int(dict1, "bar") == 23); > > if (!overwrite) { Reviewed-by: Markus Armbruster <armbru@redhat.com> Applied to qapi-next, thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-12 9:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-06 15:41 [Qemu-devel] [PATCH] tests/check-qdict: Fix missing brackets Dr. David Alan Gilbert (git) 2017-04-06 15:49 ` Eric Blake 2017-04-06 15:52 ` Dr. David Alan Gilbert 2017-04-06 15:55 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake 2017-04-07 21:27 ` Philippe Mathieu-Daudé 2017-04-12 9:04 ` [Qemu-devel] [PATCH] " Markus Armbruster
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.