Eric Blake <ebl...@redhat.com> writes:
> On 05/22/2017 11:42 AM, Markus Armbruster wrote:
>> Alternates are sum types like unions, but use the JSON type on the
>> wire / QType in QObject instead of an explicit tag. That's why we
>> require alternate members to have distinct QTypes.
>>
>> The recently introduced keyval_parse() (commit d454dbe) can only
>> produce string scalars. The qobject_input_visitor_new_keyval() input
>> visitor mostly hides the difference, so code using a QObject input
>> visitor doesn't have to care whether its input was parsed from JSON or
>> KEY=VALUE,... The difference leaks for alternates, as noted in commit
>> 0ee9ae7: an non-string, non-enum scalar alternate value can't
>
> s/an non/a non/
Will fix.
>> currently be expressed.
>>
>> In part, this is just our insufficiently sophisticated implementation.
>> Consider alternate type 'GuestFileWhence'. It has an integer member
>> and a 'QGASeek' member. The latter is an enumeration with values
>> 'set', 'cur', 'end'. The meaning of b=set, b=cur, b=end, b=0, b=1 and
>> so forth is perfectly obvious. However, our current implementation
>> falls apart at run time for b=0, b=1, and so forth. Fixable, but not
>> today; add a test case and a TODO comment.
>>
>> Now consider an alternate type with a string and an integer member.
>> What's the meaning of a=42? Is it the string "42" or the integer 42?
>> Whichever meaning you pick makes the other inexpressible. This isn't
>> just an implementation problem, it's fundamental. Our current
>> implementation will pick string.
>>
>> So far, we haven't needed such alternates. To make sure we stop and
>> think before we add one that cannot sanely work with keyval_parse(),
>> let's require alternate members to have sufficiently district
>
> s/district/distinct/
Will fix.
>> representation in KEY=VALUE,... syntax:
>>
>> * A string member clashes with any other scalar member
>>
>> * An enumeration member clashes with bool members when it has value
>> 'on' or 'off'.
>
> Does case-(in)sensitivity factor in here?
KEY=VALUE,... syntax is case-sensitive.
> Should it also be a problem
> for an enum member with value 'true' or 'false'?
No, because those are spelled "on" and "off" in KEY=VALUE,... syntax.
If we add synonyms there, we'll have to tighten the restrictions here.
>> * An enumeration member clashes with numeric members when it has a
>> value that starts with '-', '+', '0' or '9'. This is a rather lazy
>
> s/'0' or '9'/or among '0' to '9'/
Will fix.
>> approximation of the actual number syntax accepted by the visitor.
>>
>> Note that enumeration values starting with '-' and '+' are rejected
>> elsewhere already, but better safe than sorry.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>
>> +++ b/scripts/qapi.py
>> @@ -812,11 +812,26 @@ def check_alternate(expr, info):
>> if not qtype:
>> raise QAPISemError(info, "Alternate '%s' member '%s' cannot use
>> "
>> "type '%s'" % (name, key, value))
>> - if qtype in types_seen:
>> + conflicting = set([qtype])
>> + if qtype == 'QTYPE_QSTRING':
>> + enum_expr = enum_types.get(value)
>> + if enum_expr:
>> + for v in enum_expr['data']:
>> + if v in ['on', 'off']:
>
> what about 'true', 'false'?
>
> Do we care about case insensitive?
See above.
>> + conflicting.add('QTYPE_QBOOL')
>> + if re.match(r'[-+0-9.]', v): # lazy, could be tightened
>> + conflicting.add('QTYPE_QINT')
>> + conflicting.add('QTYPE_QFLOAT')
>> + else:
>> + conflicting.add('QTYPE_QINT')
>> + conflicting.add('QTYPE_QFLOAT')
>> + conflicting.add('QTYPE_QBOOL')
>> + if conflicting & set(types_seen):
>> raise QAPISemError(info, "Alternate '%s' member '%s' can't "
>> "be distinguished from member '%s'"
>> % (name, key, types_seen[qtype]))
>> - types_seen[qtype] = key
>> + for qt in conflicting:
>> + types_seen[qt] = key
>>
>
> Looks good.
Thanks!
