Re: [Qemu-devel] [PATCH v2 14/29] qapi: Concentrate QAPISchemaParser.exprs updates in .__init__()

[Markus Armbruster]

Michael Roth <address@hidden> writes:

> Quoting Markus Armbruster (2018-02-11 03:35:52)
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Marc-André Lureau <address@hidden>
>> ---
>>  scripts/qapi/common.py | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index dce289ae21..cc5a5941dd 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -290,8 +290,12 @@ class QAPISchemaParser(object):
>>                  if not isinstance(include, str):
>>                      raise QAPISemError(info,
>>                                         "Value of 'include' must be a 
>> string")
>> -                self._include(include, info, os.path.dirname(self.fname),
>> -                              previously_included)
>> +                exprs_include = self._include(include, info,
>> +                                              os.path.dirname(self.fname),
>> +                                              previously_included)
>> +                if exprs_include:
>> +                    self.exprs.extend(exprs_include.exprs)
>> +                    self.docs.extend(exprs_include.docs)
>>              elif "pragma" in expr:
>>                  self.reject_expr_doc(cur_doc)
>>                  if len(expr) != 1:
>> @@ -334,14 +338,13 @@ class QAPISchemaParser(object):
>> 
>>          # skip multiple include of the same file
>>          if incl_abs_fname in previously_included:
>> -            return
>> +            return None
>> +
>>          try:
>>              fobj = open(incl_fname, 'r')
>>          except IOError as e:
>>              raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
>> -        exprs_include = QAPISchemaParser(fobj, previously_included, info)
>> -        self.exprs.extend(exprs_include.exprs)
>> -        self.docs.extend(exprs_include.docs)
>
> minor nit, but the function of _include() seems more appropriately
> described now as _parse_include() or _parse_if_new() or something similar.

I'm open to renaming, but "parse" doesn't really fit.  _include()'s
caller checks syntax, then calls _include() to check semantic
constraints and evaluate the include, then splices in the evaluated
result.

> Maybe it would be more readable to just move the previously_included
> checks into init() and just drop _include() entirely?

Maybe.  But the conditional gets rather long then.

> Reviewed-by: Michael Roth <address@hidden>

Thanks!