Re: [Qemu-devel] [PATCH v11 21/28] qapi: Convert qtype_code into qapi enum type

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5591 bytes --]

On 11/11/2015 09:42 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> What's more meta than using qapi to define qapi? :)
>>
>> Convert qtype_code into a full-fledged[*] builtin qapi enum type,
>> so that a subsequent patch can then use it as the discriminator
>> type of qapi alternate types.  Doing so is easiest when renaming
>> it to qapi conventions, as QTypeCode.
> 
> Out of curiosity: why does the rename make the conversion easier?

It guarantees I found all affected instances.  (Although I guess the
rename could be split to a separate patch from making it builtin).

It makes sure that if we later tighten rules about naming, we won't have
to whitelist 'qtype_code' as an anomaly to our conventions.

> 
> If we rename anyway, what about renaming to QType?  Hmm, we burned that
> on a struct we use only internally in qobject/.  Oh well.

Internal structs are often easy to rename.  So if we want to avoid the
need for 'prefix', I could certainly try to achieve that (move internal
QType out of the way, then rename qtype_code to QType, then make QType
the builtin).  Looks like this one patch just became three :)

> 
>>                                        Fortunately, there are not
>> many places in the tree that were actually spelling the type name
>> out, and the judicious use of 'prefix' in the qapi defintion
> 
> definition

I've got to quit coding late at night - my rate of typos increases :)

>> +++ b/docs/qapi-code-gen.txt
>> @@ -163,6 +163,7 @@ The following types are predefined, and map to C as follows:
>>                         accepts size suffixes
>>    bool      bool       JSON true or false
>>    any       QObject *  any JSON value
>> +  QTypeCode QTypeCode  JSON string of enum QTypeCode values
> 
> QTypeCode is currently used only internally, so the JSON values don't
> matter.  I don't expect that to change.  However, we either enforce
> internal use somehow, or document the JSON values.  Documenting them is
> easier.
> 
> In short, your patch is fine.
> 

>> -
>> -struct QObject;
>> +#include "qapi-types.h"
>>
>>  typedef struct QType {
>> -    qtype_code code;
>> +    QTypeCode code;
>>      void (*destroy)(struct QObject *);
>>  } QType;
>>
>    typedef struct QObject {
>        const QType *type;
>        size_t refcnt;
>    } QObject;
> 
> Note: typedef name QObject still defined here.

Oh, I see what you're saying. Since qapi-types.h now has a forward
declaration of the QObject typedef, this could be changed to just

struct QObject {
...
};

>> +++ b/scripts/qapi-types.py
>> @@ -233,8 +233,14 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>          self.defn += gen_type_cleanup(name)
>>
>>      def visit_enum_type(self, name, info, values, prefix):
>> -        self._fwdecl += gen_enum(name, values, prefix)
>> -        self._fwdefn += gen_enum_lookup(name, values, prefix)
>> +        # Special case for our lone builtin enum type
>> +        if name == 'QTypeCode':
> 
> Would "if not info" work?  Same in qapi-visit.py below.

Feels a bit hacky, since we just recently added is_implicit() to hide
(and then change) the 'if not info' check on objects.  Maybe an accessor
is_builtin() makes more sense?  But yes, same approach to both client files.


>> -#include "qapi/qmp/qobject.h"
>> +
>> +typedef struct QObject QObject;
> 
> Typedef name QObject now also defined here.  GCC accepts this silently
> without -Wpedantic, but other compilers might not.  Whether we care for
> such compilers or not, defining things in exactly one place is neater.
> 
> Possible fixes:
> 
> * Drop the typedef from qobject.h
> 
> * Don't add it to qapi-types.h, and use struct QObject there
> 

I favor dropping the second typedef.


>> +++ b/scripts/qapi.py
>> @@ -33,7 +33,7 @@ builtin_types = {
>>      'uint32':   'QTYPE_QINT',
>>      'uint64':   'QTYPE_QINT',
>>      'size':     'QTYPE_QINT',
>> -    'any':      None,           # any qtype_code possible, actually
>> +    'any':      None,           # any QTypeCode possible, actually
>>  }
>>
> 
> Should we list QTypeCode here?

Yeah, probably.  This array is only used by the ad hoc parser, and may
disappear later as we move more into check(), but we should be
consistent in the meantime.

> 
>>  # Whitelist of commands allowed to return a non-dictionary
>> @@ -1243,6 +1243,11 @@ class QAPISchema(object):
>>          self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
>>                                                            [], None)
>>          self._def_entity(self.the_empty_object_type)
>> +        self._def_entity(QAPISchemaEnumType('QTypeCode', None,
>> +                                            ['none', 'qnull', 'qint',
>> +                                             'qstring', 'qdict', 'qlist',
>> +                                             'qfloat', 'qbool'],
>> +                                            'QTYPE'))
>>
>>      def _make_implicit_enum_type(self, name, info, values):
>>          name = name + 'Kind'   # Use namespace reserved by add_name()
> [Trivial changes to expected test output snipped]

I debated about hacking tests/qapi-schema/test-qapi.py to omit QTypeCode
(the way we already omit builtin types and things like 'intList'), for
less churn in the .out files.  I can go either way, if you have a
preference.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]