David Hildenbrand <da...@redhat.com> writes:
> On 15.11.18 10:48, Markus Armbruster wrote:
>> David Hildenbrand <da...@redhat.com> writes:
>>
>>> On 14.11.18 18:38, Markus Armbruster wrote:
>>>> David Hildenbrand <da...@redhat.com> writes:
>>>>
>>>>> The input visitor has some problems right now, especially
>>>>> - unsigned type "Range" is used to process signed ranges, resulting in
>>>>> inconsistent behavior and ugly/magical code
>>>>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>>>>> supported and error messages are misleading
>>>>> - lists/ranges of int64_t are accepted although no list is parsed and
>>>>> we should rather report an error
>>>>> - lists/ranges are preparsed using int64_t, making it hard to
>>>>> implement uint64_t values or uint64_t lists
>>>>> - types that don't support lists don't bail out
>>>>
>>>> Known weirdness: empty list is invalid (test-string-input-visitor.c
>>>> demonstates). Your patch doesn't change that (or else it would update
>>>> the test). Should it be changed?
>>>>
>>>
>>> I don't change the test, so the old behavior still works.
>>> (empty string -> error)
>>
>> Understand. Design question: should it remain an error? Feel free to
>> declare the question out of scope for this patch.
>
> I think I was confused, let me retry to explain.
>
> Empty lists actually don't result in an error. Calling start_list() on
> an empty string works just fine.
>
> However
> - check_list() will result in "Fewer list elements expected"
> - visit_type_.*int64() will result in "Fewer list elements expected"
> - next_list() will result in NULL
>
> I guess that is the intended behavior. E.g. the test does
>
> v = visitor_input_test_init(data, "");
> visit_type_uint64List(v, NULL, &res, &error_abort);
> g_assert(!res);
>
> So there won't be any error as the first "visit_next_list()" will
> properly indicate "NULL".
You know, I was confused, too :) I looked at commit 3d089cea0d3, which
added the test case:
+ /* Empty list is invalid (weird) */
+
+ v = visitor_input_test_init(data, "");
+ visit_type_int64List(v, NULL, &res, &err);
+ error_free_or_abort(&err);
I missed regression fix commit d2788227c61:
- /* Empty list is invalid (weird) */
+ /* Empty list */
v = visitor_input_test_init(data, "");
- visit_type_int64List(v, NULL, &res, &err);
- error_free_or_abort(&err);
+ visit_type_int64List(v, NULL, &res, &error_abort);
+ g_assert(!res);
So the test actually demonstrates empty lists work fine before and after
your patch.
>>> Added "Only flat lists of integers (int64/uint64) are supported."
>>
>> Hmm, do lists of narrower integer types also work? I guess they do: the
>> narrower visit_type_*int*() call v->type_*int64() via
>> visit_type_*intN().
>>
>> Lists of type size are expressly excluded, in parse_type_size() below.
>> That's okay, we can lift the restriction when it gets in the way.
>
> Right, we can make that clearer
>
> "Only flat lists of integers (except type "size") are supported." ?
> [...]
Sold!
[...]
