[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alte
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates |
Date: |
Mon, 28 Sep 2015 11:26:19 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 09/24/2015 10:29 AM, Markus Armbruster wrote:
>
>>>>> +
>>>>> + /* FIXME: Order of alternate should not affect semantics */
>>>>
>>>> Inhowfar does it affect semantics? Or asked differently: what exactly
>>>> is wrong with this test now?
>>>>
>>>>> + v = visitor_input_test_init(data, "42");
>>>>> + visit_type_AltThree(v, &three, NULL, &error_abort);
>>>>> + g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N);
>>>>> + g_assert_cmpfloat(three->n, ==, 42);
>>>>> + qapi_free_AltThree(three);
>>>>> + one = NULL;
>>>
>>>
>>> AltTwo and AltThree are ostensibly the same struct (two branches, one
>>> for 'str' and one for 'number', just in a different order), but they
>>> parsed differently (AltTwo failed, AltThree succeeded). The bug is
>>> fixed later when the order of the branch declaration no longer impacts
>>> the result of the parse.
>>
>> Then nothing is wrong with this test case, and the FIXME doesn't belong
>> here.
>
> Actually, the test for AltThree succeeds only by accident. There are two
> bugs at play; when I fix the first bug (order shouldn't matter: AltTwo
> and AltThree should parse identically), then the second bug is finally
> exposed (integers aren't being parsed as numbers, in either AltTwo or
> AltThree). But I can certainly rework the FIXMEs both here and on the
> first fix (19/46) to make it more obvious what the second fix (20/46) is
> good for.
Yes, please. I find the FIXME quoted above confusing, because it makes
me look for problems exposed by this test when there are none.
- Re: [Qemu-devel] [PATCH v5 04/46] qapi: Add tests for empty unions, (continued)
[Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/21
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/22
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/22
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/23
[Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8, Eric Blake, 2015/09/21