Re: [Qemu-devel] [PATCH v6 15/27] qapi: rename allow_dict to allow_implicit

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> On Wed, Dec 5, 2018 at 10:41 PM Markus Armbruster <address@hidden> wrote:
>>
>> Marc-André Lureau <address@hidden> writes:
>>
>> > This makes it a bit clearer what is the intent of the dictionnary for
>>
>> dictionary
>
> sigh, this must be a very common misspell (dictionnaire in french)

Muscle memory...

>> > the check_type() function, since there was some confusion on a
>> > previous iteration of this series.
>>
>> I don't remember... got a pointer to that confusion handy?
>
> https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01584.html

Thanks!

My review comment was

    > diff --git a/scripts/qapi.py b/scripts/qapi.py
    > index df2a304e8f..15711f96fa 100644
    > --- a/scripts/qapi.py
    > +++ b/scripts/qapi.py
    > @@ -696,7 +696,15 @@ def check_type(info, source, value, 
allow_array=False,
    >          return
    >  
    >      if not allow_dict:
    > -        raise QAPISemError(info, "%s should be a type name" % source)
    > +        if isinstance(value, dict) and 'type' in value:
    > +            check_type(info, source, value['type'], allow_array,
    > +                       allow_dict, allow_optional, allow_metas)
    > +            if 'if' in value:
    > +                check_if(value, info)
    > +            check_unknown_keys(info, value, {'type', 'if'})
    > +            return
    > +        else:
    > +            raise QAPISemError(info, "%s should be a type name" % source)

    @allow_dict becomes a misnomer: dictionaries are now always allowed, but
    they have different meaning (implicit type vs. named type with
    additional attributes).

    Rename to @allow_implicit?

As far as I can tell, it's not an issue in the current version of your
patches anymore:

    def check_type(info, source, value, allow_array=False,
                   allow_implicit=False, allow_optional=False,
                   allow_metas=[]):
        global all_names

        if value is None:
            return

        # Check if array type for value is okay
        if isinstance(value, list):
            if not allow_array:
                raise QAPISemError(info, "%s cannot be an array" % source)
            if len(value) != 1 or not isinstance(value[0], str):
                raise QAPISemError(info,
                                   "%s: array type must contain single type 
name" %
                                   source)
            value = value[0]

        # Check if type name for value is okay
        if isinstance(value, str):
            if value not in all_names:
                raise QAPISemError(info, "%s uses unknown type '%s'"
                                   % (source, value))
            if not all_names[value] in allow_metas:
                raise QAPISemError(info, "%s cannot use %s type '%s'" %
                                   (source, all_names[value], value))
            return

        if not allow_implicit:
            raise QAPISemError(info, "%s should be a type name" % source)

        if not isinstance(value, OrderedDict):
            raise QAPISemError(info,
                               "%s should be a dictionary or type name" % 
source)

        # value is a dictionary, check that each member is okay
        for (key, arg) in value.items():
            check_name(info, "Member of %s" % source, key,
                       allow_optional=allow_optional)
            if c_name(key, False) == 'u' or c_name(key, 
False).startswith('has_'):
                raise QAPISemError(info, "Member of %s uses reserved name '%s'"
                                   % (source, key))
            # Todo: allow dictionaries to represent default values of
            # an optional argument.
            if isinstance(arg, dict):
                check_known_keys(info, "member '%s' of %s" % (key, source),
                                 arg, ['type'], ['if'])
                typ = arg['type']
            else:
                typ = arg
            check_type(info, "Member '%s' of %s" % (key, source),
                       typ, allow_array=True,
                       allow_metas=['built-in', 'union', 'alternate', 'struct',
                                    'enum'])


>> > Suggested-by: Markus Armbruster <address@hidden>
>> > Signed-off-by: Marc-André Lureau <address@hidden>