Max Reitz <mre...@redhat.com> writes:
> On 14.11.19 10:15, Markus Armbruster wrote:
>> Max Reitz <mre...@redhat.com> writes:
>>
>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>> ---
>>> 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.
I'm totally cool with deviating from JSON, all I really care about is
proper schema language documentation.
If we stick to JSON form number syntax, this is easy: replace "Numbers
and null are not supported" by "null is not supported", done.
Implementation should also be easy enough: convert RFC 8259's EBNF to a
regexp, feed the matched string to Python's number parser, done.
Fancier syntax we'd need to document ourselves. I'm willing to deal
with that if we have a sufficiently compelling use for them.
>> 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.
Ah, now I see.
> 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.
Let's start simple & stupid. Extensions can be added on top.
Hexadecimal integers may well be compelling enough to justify 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.
The test name 'bad-type-int' is now bad, because the int in it is no
longer bad (pardon my lame puns).
> Thanks for reviewing!
Better late than never, I guess... You're welcome!
>
> 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"
