Eric Blake <ebl...@redhat.com> writes:
> On 09/24/2015 08:36 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> writes:
>>
>>> Add some testsuite exposure for use of a 'number' as part of
>>> an alternate. The current state of the tree has a few bugs
>>> exposed by this: our input parser depends on the ordering of
>>> how the qapi schema declared the alternate, and the parser
>>> does not accept integers for a 'number' in an alternate even
>>> though it does for numbers outside of an alternate.
>>>
>>> Mixing 'int' and 'number' in the same alternate is unusual,
>>> since both are supplied by json-numbers, but there does not
>>> seem to be a technical reason to forbid it given that our
>>> json lexer distinguishes between json-numbers that can be
>>> represented as an int vs. those that cannot.
>>>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>> ---
>>> tests/qapi-schema/qapi-schema-test.json | 8 ++
>>> tests/qapi-schema/qapi-schema-test.out | 24 ++++++
>>> tests/test-qmp-input-visitor.c | 129
>>> +++++++++++++++++++++++++++++++-
>>> 3 files changed, 158 insertions(+), 3 deletions(-)
>>>
>
>>> +++ b/tests/test-qmp-input-visitor.c
>>> @@ -368,15 +368,136 @@ static void
>>> test_visitor_in_alternate(TestInputVisitorData *data,
>>> {
>>> Visitor *v;
>>> Error *err = NULL;
>>> - UserDefAlternate *tmp;
>>> + UserDefAlternate *tmp = NULL;
>>
>> Any particular reason for adding the initializer?
>>
>>>
>>> v = visitor_input_test_init(data, "42");
>>>
>>> - visit_type_UserDefAlternate(v, &tmp, NULL, &err);
>>> - g_assert(err == NULL);
>>> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
>
> Hmm - I don't know if we have a clear contract for what happens if you
> call visit_type_FOO on an uninitialized pointer. It may have been
> succeeding by mere luck.
I strongly suspect the "input" visitors are assignment-like: they store
something, and don't care what value they overwrite. Let's keep that in
mind when we retrofit a contract.
>>
>> The pattern
>>
>> foo(..., &err);
>> g_assert(err == NULL);
>>
>> is pretty common in tests. Can't see what it buys us over straight
>> &error_abort. Perhaps I'll spatch it away.
>>
>>> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>>> g_assert_cmpint(tmp->i, ==, 42);
>>> qapi_free_UserDefAlternate(tmp);
>>> + tmp = NULL;
>>
>> Why do you need to clear tmp?
>
> If we were succeeding on a single call by mere luck where tmp started
> life as all 0 due to stack contents, but the second call has tmp
> pointing to stale memory, then that would be an obvious reason. I'll
> have to revisit what happens, because I don't recall any specific reason
> for why I did this other than the symmetry of making sure each parse had
> clean state (that is, I don't recall a crash happening if I didn't do
> it, and haven't yet tested under valgrind to see if we are provably
> using memory incorrectly if we don't initialize).
I suspect it's just as dead as clearing an assignment's LHS before the
actual assignment is :)
>>> +
>>> + /* FIXME: Integers should parse as numbers */
>>
>> Suggest to augment or replace this comment...
>>
>>> + v = visitor_input_test_init(data, "42");
>>> + visit_type_AltTwo(v, &two, NULL, &err);
>>
>> ... with
>>
>> /* FIXME g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); */
>> /* FIXME g_assert_cmpfloat(two->n, ==, 42); */
>
> Ah, to better document what the test will look like in the future when
> the bugs are fixed. Sure, I can do that.
>
>>
>>> + g_assert(err);
>>> + error_free(err);
>>> + err = NULL;
>>> + qapi_free_AltTwo(two);
>>> + one = NULL;
>>
>> *chuckle* Why do you clear one here? More of the same below.
>
> Too much copy-and-paste. Will fix.
Supports my idea that these assignments are useless.
If they are, let's just drop them.
>>> +
>>> + /* 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.
>> Reading this, I had to refer back to the definition of AltOne, ...,
>> AltSix all the time. Let's rename them to AltStrBool, AltStrNum, ...,
>> AltNumInt.
>
> Good idea, will do.
