[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 01/14] qapi: Parse numeric values
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v4 01/14] qapi: Parse numeric values |
Date: |
Thu, 14 Nov 2019 10:50:10 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
On 14.11.19 10:15, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>> tests/qapi-schema/bad-type-int.json | 1 -
>> tests/qapi-schema/enum-int-member.json | 1 -
>> scripts/qapi/common.py | 25 ++++++++++++++++++++----
>> scripts/qapi/introspect.py | 2 ++
>> tests/qapi-schema/bad-type-int.err | 2 +-
>> tests/qapi-schema/enum-int-member.err | 2 +-
>> tests/qapi-schema/leading-comma-list.err | 2 +-
>> 7 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/qapi-schema/bad-type-int.json
>> b/tests/qapi-schema/bad-type-int.json
>> index 56fc6f8126..81355eb196 100644
>> --- a/tests/qapi-schema/bad-type-int.json
>> +++ b/tests/qapi-schema/bad-type-int.json
>> @@ -1,3 +1,2 @@
>> # we reject an expression with a metatype that is not a string
>> -# FIXME: once the parser understands integer inputs, improve the error
>> message
>> { 'struct': 1, 'data': { } }
>> diff --git a/tests/qapi-schema/enum-int-member.json
>> b/tests/qapi-schema/enum-int-member.json
>> index 6c9c32e149..6958440c6d 100644
>> --- a/tests/qapi-schema/enum-int-member.json
>> +++ b/tests/qapi-schema/enum-int-member.json
>> @@ -1,3 +1,2 @@
>> # we reject any enum member that is not a string
>> -# FIXME: once the parser understands integer inputs, improve the error
>> message
>> { 'enum': 'MyEnum', 'data': [ 1 ] }
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index d61bfdc526..3396ea4a09 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -498,6 +498,8 @@ class QAPISchemaParser(object):
>> raise QAPISemError(info, "Unknown pragma '%s'" % name)
>>
>> def accept(self, skip_comment=True):
>> + num_match = re.compile(r'([-+]?inf|nan|[-+0-9.][0-9a-f.ex]*)')
>> +
>> while True:
>> self.tok = self.src[self.cursor]
>> self.pos = self.cursor
>
> This is yet another extension over plain JSON. RFC 8259:
>
> number = [ minus ] int [ frac ] [ exp ]
> decimal-point = %x2E ; .
> digit1-9 = %x31-39 ; 1-9
> e = %x65 / %x45 ; e E
> exp = e [ minus / plus ] 1*DIGIT
> frac = decimal-point 1*DIGIT
> int = zero / ( digit1-9 *DIGIT )
> minus = %x2D ; -
> plus = %x2B ; +
> zero = %x30 ; 0
>
> Extensions are acceptable when we have an actual use for it, and we
> document them properly.
Well, it isn’t really an extension, because this isn’t a JSON parser but
just something that accepts anything that looks like a number and then
lets Python try a conversion on it.
> Isn't the parenthesis in your regular expression redundant?
You’re right, but on second thought, maybe I should surround it by \<
and \>.
> What use do you have in mind for 'inf' and 'nan'?
I could imagine inf being a useful default value, actually. nan,
probably not so much.
> Why accept leading '+' as in '+123'?
>
> Why accept empty integral part as in '.123'?
>
> Why accept '.xe.'? Kidding you, that must be a bug in your regexp.
Well, kind of.
I wanted to accept anything that looks in any way like a number and then
let Python try to convert it. That’s also the reason why the case comes
last.
For that reason, I decided to keep the regex as simple as possible,
because the attempted conversions would reject anything that isn’t (to
Python) a valid number later.
It was my impression that the QAPI schema isn’t really JSON anyway and
that our QAPI schema parser isn’t a JSON parser. Under that assumption
it simply seemed useful to me to accept anything that could potentially
be a number to Python and convert it.
Now, honestly, I still don’t see the point of having a strict JSON
“parser” here, but if you insist. Seems possible to do in a regex.
Though I do think it makes sense to support hex integers as an extension.
> Please decide what number syntax you'd like to accept, then specify it
> in docs/devel/qapi-code-gen.txt, so we can first discuss the
> specification, and then check the regexp implements it.
>
> docs/devel/qapi-code-gen.txt update goes here:
>
> === Schema syntax ===
>
> Syntax is loosely based on JSON (http://www.ietf.org/rfc/rfc8259.txt).
> Differences:
>
> * Comments: start with a hash character (#) that is not part of a
> string, and extend to the end of the line.
>
> * Strings are enclosed in 'single quotes', not "double quotes".
>
> * Strings are restricted to printable ASCII, and escape sequences to
> just '\\'.
>
> --> * Numbers and null are not supported.
OK.
> Hrmm, commit 9d55380b5a "qapi: Remove null from schema language" left
> two instances in error messages behind. I'll fix them.
>
>> @@ -584,7 +586,22 @@ class QAPISchemaParser(object):
>> return
>> self.line += 1
>> self.line_pos = self.cursor
>> - elif not self.tok.isspace():
>> + elif self.tok.isspace():
>> + pass
>> + elif num_match.match(self.src[self.pos:]):
>> + match = num_match.match(self.src[self.pos:]).group(0)
>
> Sadly, the walrus operator is Python 3.8.
>
>> + try:
>> + self.val = int(match, 0)
>> + except ValueError:
>> + try:
>> + self.val = float(match)
>> + except ValueError:
>> + raise QAPIParseError(self,
>> + '"%s" is not a valid integer or float' %
>> match)
>> +
>> + self.cursor += len(match) - 1
>> + return
>> + else:
>> raise QAPIParseError(self, 'Stray "%s"' % self.tok)
>
> Any particular reason for putting the number case last?
Because the match is so broad.
>>
>> def get_members(self):
>> @@ -617,9 +634,9 @@ class QAPISchemaParser(object):
>> if self.tok == ']':
>> self.accept()
>> return expr
>> - if self.tok not in "{['tfn":
>> + if self.tok not in "{['tfn-+0123456789.i":
>
> This is getting a bit ugly. Let's not worry about it now.
>
>> raise QAPIParseError(self, 'Expected "{", "[", "]", string, '
>> - 'boolean or "null"')
>> + 'boolean, number or "null"')
>> while True:
>> expr.append(self.get_expr(True))
>> if self.tok == ']':
>> @@ -638,7 +655,7 @@ class QAPISchemaParser(object):
>> elif self.tok == '[':
>> self.accept()
>> expr = self.get_values()
>> - elif self.tok in "'tfn":
>> + elif self.tok in "'tfn-+0123456789.i":
>> expr = self.val
>> self.accept()
>> else:
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index f62cf0a2e1..6a61dd831f 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -57,6 +57,8 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>> ret += indent(level) + '}))'
>> elif isinstance(obj, bool):
>> ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
>> + elif isinstance(obj, int) and obj >= -(2 ** 63) and obj < 2 ** 63:
>> + ret += 'QLIT_QNUM(%i)' % obj
>
> Please explain the range check.
Will do.
>> else:
>> assert False # not implemented
>> if level > 0:
>> diff --git a/tests/qapi-schema/bad-type-int.err
>> b/tests/qapi-schema/bad-type-int.err
>> index da89895404..e22fb4f655 100644
>> --- a/tests/qapi-schema/bad-type-int.err
>> +++ b/tests/qapi-schema/bad-type-int.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/bad-type-int.json:3:13: Stray "1"
>> +tests/qapi-schema/bad-type-int.json:2: 'struct' key must have a string value
>
> Test needs a rename, assuming it's not redundant now.
I’m not adding a test here, it’s just the value has changed in
4d42815587063d.
Thanks for reviewing!
Max
>> diff --git a/tests/qapi-schema/enum-int-member.err
>> b/tests/qapi-schema/enum-int-member.err
>> index 071c5213d8..112175f79d 100644
>> --- a/tests/qapi-schema/enum-int-member.err
>> +++ b/tests/qapi-schema/enum-int-member.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/enum-int-member.json:3:31: Stray "1"
>> +tests/qapi-schema/enum-int-member.json:2: Member of enum 'MyEnum' requires
>> a string name
>
> This one's name is still good.
>
>> diff --git a/tests/qapi-schema/leading-comma-list.err
>> b/tests/qapi-schema/leading-comma-list.err
>> index f5c870bb9c..fa9c80aa57 100644
>> --- a/tests/qapi-schema/leading-comma-list.err
>> +++ b/tests/qapi-schema/leading-comma-list.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/leading-comma-list.json:2:13: Expected "{", "[", "]",
>> string, boolean or "null"
>> +tests/qapi-schema/leading-comma-list.json:2:13: Expected "{", "[", "]",
>> string, boolean, number or "null"
signature.asc
Description: OpenPGP digital signature