This makes it a bit clearer what is the intent of the dictionnary for
the check_type() function, since there was some confusion on a
previous iteration of this series.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/common.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 95b7cd74ee..d83fa1900e 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -661,7 +661,7 @@ def check_if(expr, info):
 
 
 def check_type(info, source, value, allow_array=False,
-               allow_dict=False, allow_optional=False,
+               allow_implicit=False, allow_optional=False,
                allow_metas=[]):
     global all_names
 
@@ -688,7 +688,7 @@ def check_type(info, source, value, allow_array=False,
                                (source, all_names[value], value))
         return
 
-    if not allow_dict:
+    if not allow_implicit:
         raise QAPISemError(info, "%s should be a type name" % source)
 
     if not isinstance(value, OrderedDict):
@@ -718,7 +718,7 @@ def check_command(expr, info):
     if boxed:
         args_meta += ['union', 'alternate']
     check_type(info, "'data' for command '%s'" % name,
-               expr.get('data'), allow_dict=not boxed, allow_optional=True,
+               expr.get('data'), allow_implicit=not boxed, allow_optional=True,
                allow_metas=args_meta)
     returns_meta = ['union', 'struct']
     if name in returns_whitelist:
@@ -736,7 +736,7 @@ def check_event(expr, info):
     if boxed:
         meta += ['union', 'alternate']
     check_type(info, "'data' for event '%s'" % name,
-               expr.get('data'), allow_dict=not boxed, allow_optional=True,
+               expr.get('data'), allow_implicit=not boxed, allow_optional=True,
                allow_metas=meta)
 
 
@@ -764,7 +764,7 @@ def check_union(expr, info):
     else:
         # The object must have a string or dictionary 'base'.
         check_type(info, "'base' for union '%s'" % name,
-                   base, allow_dict=True, allow_optional=True,
+                   base, allow_implicit=True, allow_optional=True,
                    allow_metas=['struct'])
         if not base:
             raise QAPISemError(info, "Flat union '%s' must have a base"
@@ -888,7 +888,7 @@ def check_struct(expr, info):
     members = expr['data']
 
     check_type(info, "'data' for struct '%s'" % name, members,
-               allow_dict=True, allow_optional=True)
+               allow_implicit=True, allow_optional=True)
     check_type(info, "'base' for struct '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
 
-- 
2.18.0.rc1

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> This makes it a bit clearer what is the intent of the dictionnary for

dictionary

> 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?

> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

On Wed, Dec 5, 2018 at 10:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 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)

>
> > 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

>
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> On Wed, Dec 5, 2018 at 10:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <armbru@redhat.com>
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi
On Mon, Dec 10, 2018 at 12:51 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > On Wed, Dec 5, 2018 at 10:41 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <armbru@redhat.com>
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Feel free to drop it (allow_implicit seems a bit more clear to me though).

-- 
Marc-André Lureau