| Message ID | 20170406154107.9178-1-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
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) 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
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.
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>
"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!
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) {