Re: [Qemu-devel] [PATCH v4 06/51] qapi: pass 'if' condition into QAPISchemaEntity objects

[Markus Armbruster]

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

> Hi
>
> On Tue, Feb 6, 2018 at 11:12 AM, Markus Armbruster <address@hidden> wrote:
>> Marc-André Lureau <address@hidden> writes:
>>
>>> Built-in objects remain unconditional.  Explicitly defined objects
>>> use the condition specified in the schema.  Implicitly defined
>>> objects inherit their condition from their users.  For most of them,
>>> there is exactly one user, so the condition to use is obvious.  The
>>> exception is the wrapped type's generated for simple union variants,
>>> which can be shared by any number of simple unions.  The tight
>>> condition would be the disjunction of the conditions of these simple
>>> unions.  For now, use wrapped type's condition instead.  Much
>>> simpler and good enough for now.
>>>
>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>> Reviewed-by: Markus Armbruster <address@hidden>
>>> ---
>>>  scripts/qapi.py | 98 
>>> ++++++++++++++++++++++++++++++++++++++-------------------
>>>  1 file changed, 66 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 27df0fcf48..8f54dead8d 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -991,8 +991,17 @@ def check_exprs(exprs):
>>>  # Schema compiler frontend
>>>  #
>>>
>>> +def listify_cond(ifcond):
>>> +    if not ifcond:
>>> +        return []
>>> +    elif not isinstance(ifcond, list):
>>> +        return [ifcond]
>>> +    else:
>>> +        return ifcond
>>
>> pylint complains:
>>
>>     R:995, 4: Unnecessary "else" after "return" (no-else-return)
>>
>> Matter of taste.  Mine happens to agree with pylint's.  Suggest:
>>
>>    def listify_cond(ifcond):
>>        if not ifcond:
>>            return []
>>        if not isinstance(ifcond, list):
>>            return [ifcond]
>>        return ifcond
>>
>
> There are so many errors with pylint, that I don't bother running it.

pylint reports lots of stuff that is actually just fine.

> How did you notice? you run diff of error output between commits?

Yes.

> pycodestyle --diff is more convenient, and silent on this code.

Formerly known as pep8.  Confusingly, Fedora 26 packages both
separately.  Thanks for the pointer.

> Feel free to touch if you pick the patch.

Okay.

>>> +
>>> +
>>>  class QAPISchemaEntity(object):
>>> -    def __init__(self, name, info, doc):
>>> +    def __init__(self, name, info, doc, ifcond=None):
>>>          assert isinstance(name, str)
>>>          self.name = name
>>>          # For explicitly defined entities, info points to the (explicit)
>> [...]
>>