Re: [Qemu-devel] [PATCH v2 14/29] qapi: Concentrate QAPISchemaParser.exprs updates in .__init__()
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__()
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__()
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