Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/23/2014 08:56 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> The previous commit demonstrated that the generator overlooked some
>>> fairly basic broken expressions:
>>> - missing metataype
>>> - metatype key has a non-string value
>>> - unknown key in relation to the metatype
>>> - conflicting metatype (this patch treats the second metatype as an
>>> unknown key of the first key visited, which is not necessarily the
>>> first key the user typed)
>> 
>> The whole thing is a Saturday afternoon hack, with extra hacks bolted on
>> left & right.
>
> And this series is a sequence of MY Saturday afternoon hacks in cleaning
> it up :)

Much appreciated!

>>> Add check_keys to cover these situations, and update testcases to
>>> match.  A couple other tests (enum-missing-data, indented-expr) had
>>> to change since the validation added here occurs so early.
>>>
>>> While valid .json files won't trigger any of these cases, we might
>>> as well be nicer to developers that make a typo while trying to add
>>> new QAPI code.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>> 
>
>>> +def check_keys(expr_elem, meta, required, optional=[]):
>>> +    expr = expr_elem['expr']
>>> +    info = expr_elem['info']
>>> +    name = expr[meta]
>> 
>> Caller ensures expr[meta] exists.  Okay.
>> 
>>> +    if not isinstance(name, basestring):
>> 
>> I'm a Python noob: why basestring and not str?
>
> Me too.  No clue.  Copy and paste from existing code.
> http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361

Yes.  It's our only use of basestring.  Other places use isinstance(FOO,
str).

The basestring use comes from Kevin's commit b35284e.  Kevin, why
basestring and not str?

>> 
>>> +        raise QAPIExprError(info,
>>> +                            "%s key must have a string value" % meta)
>>> +    expr_elem['name'] = name
>> 
>> Where is this used?
>
> Hmm, I know I used it at one point in my series (to be able to print the
> name of an expression in check_type added in 13/19, without having to
> repeat the dance of if enum: expr['enum'] elif union: expr['union'] etc.
> in multiple places).  Although in my refactoring, I may have eliminated
> the need for it after all.  If it's not in use at the end of the series,
> I can drop it.

A quick grep comes up empty.

>>> +    required.append(meta)
>> 
>> Ugly side effect.  To avoid, either make a new list here
>> 
>>     required = required + [ meta ]
>> 
>> or do nothing here and...
>> 
>>> +    for (key, value) in expr.items():
>>> +        if not key in required and not key in optional:
>> 
>> ... add "and key != meta" to this condition.
>
> Shows my inexperience in python.  Sure, I can fix.  Looks like I get to
> spin a v5.
>
>> 
>> This hunk is easier to review with whitespace ignored:
>
> Indeed. Alas, python is a stickler about whitespace reindentation.
> Since I have to respin, I'll probably split into two pieces (one with a
> no-op change that reindents, the other for the improvements).

Not necessary for me, I'm comfy with telling Emacs to hide whitespace
differences :)