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

[Markus Armbruster]

Michael Roth  writes:

> Quoting Markus Armbruster (2018-02-11 03:35:52)
>> Signed-off-by: Markus Armbruster 
>> Reviewed-by: Marc-André Lureau 
>> ---
>>  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 

Thanks!

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

[Michael Roth]

Quoting Markus Armbruster (2018-02-11 03:35:52)
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Marc-André Lureau 
> ---
>  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.
Maybe it would be more readable to just move the previously_included
checks into init() and just drop _include() entirely?

Reviewed-by: Michael Roth 

> +return QAPISchemaParser(fobj, previously_included, info)
> 
>  def _pragma(self, name, value, info):
>  global doc_required, returns_whitelist, name_case_whitelist
> -- 
> 2.13.6
> 

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

[Eric Blake]

On 02/11/2018 03:35 AM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
Reviewed-by: Marc-André Lureau 
---
  scripts/qapi/common.py | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)


Again, not much mention of 'why' in the commit message.  But 
centralizing the initialization operations in one place rather than 
having to chase down multiple places is good.


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org