tests/check-qdict: Fix missing brackets

PATCH Applied Dr. David Alan Gilbert (git) 4 participants 6 messages Apr 6, 2017 – Apr 12, 2017

lore

Share & tools

lore

Dr. David Alan Gilbert (git)

author

2017-04-06 15:41 UTC #0 lore raw

PATCH 1/1 tests/check-qdict: Fix missing brackets

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

---
 tests/check-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tests/check-qdict.c +1 -1

index 81162ee572..6f3fbcf9c1 100644
@@ -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) {

Signature · 1 line

2.12.2

Eric Blake

2017-04-06 15:50 UTC #1 lore raw

Quoted 15 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/check-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/check-qdict.c +0 -0
index 81162ee572..6f3fbcf9c1 100644

tests/check-qdict.c static void qdict_join_test(void) · line 562

Quoted 5 lines

tests/check-qdict.c +1 -1

@@ -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?

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Dr. David Alan Gilbert

author

2017-04-06 15:53 UTC #2 lore raw

Quoted 19 lines · * Eric Blake ([email protected]) wrote:

Quoted 18 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/check-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/check-qdict.c +0 -0
index 81162ee572..6f3fbcf9c1 100644
@@ -559,7 +559,7 @@ static void qdict_join_test(void)
         g_assert(qdict_size(dict1) == 2);
         g_assert(qdict_size(dict2) == !overwrite);
 

Quoted 5 lines

tests/check-qdict.c static void qdict_join_test(void) · line 562
Quoted 2 lines
tests/check-qdict.c +1 -1
-        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

Quoted 4 lines

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Eric Blake

Reviewed-by

2017-04-06 15:55 UTC #3 lore raw

In reply to Eric Blake

Quoted 19 lines · Eric Blake

Quoted 18 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/check-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/check-qdict.c +0 -0
index 81162ee572..6f3fbcf9c1 100644
@@ -559,7 +559,7 @@ static void qdict_join_test(void)
         g_assert(qdict_size(dict1) == 2);
         g_assert(qdict_size(dict2) == !overwrite);
 

Quoted 5 lines

tests/check-qdict.c static void qdict_join_test(void) · line 562
Quoted 2 lines
tests/check-qdict.c +1 -1
-        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 <[email protected]>

Safe for 2.9, but late enough that it also doesn't matter if it slips until 2.10.

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Philippe Mathieu-Daudé

Reviewed-by

2017-04-07 21:27 UTC #4 lore raw

Quoted 43 lines · Eric Blake

Quoted 19 lines · Eric Blake
Quoted 18 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/check-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/check-qdict.c +0 -0
index 81162ee572..6f3fbcf9c1 100644
@@ -559,7 +559,7 @@ static void qdict_join_test(void)
         g_assert(qdict_size(dict1) == 2);
         g_assert(qdict_size(dict2) == !overwrite);
 
Quoted 5 lines
tests/check-qdict.c static void qdict_join_test(void) · line 562
Quoted 2 lines
tests/check-qdict.c +1 -1
-        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)

Quoted 7 lines

which is then asserting that we actually have the value we expect.
Reviewed-by Eric Blake <[email protected]>
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é <[email protected]>

Markus Armbruster

Reviewed-by on v1 1/1 Applied

2017-04-12 09:04 UTC #5 lore raw

In reply to Dr. David Alan Gilbert (git)

Quoted 15 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>
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 <[email protected]>
---
 tests/check-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tests/check-qdict.c +0 -0
index 81162ee572..6f3fbcf9c1 100644

tests/check-qdict.c static void qdict_join_test(void) · line 565

Quoted 8 lines

tests/check-qdict.c +1 -1

@@ -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 <[email protected]>

Applied to qapi-next, thanks!