Re: [PATCH v2 04/21] qapi/parser: factor parsing routine into method

[John Snow]

On 5/18/21 5:57 AM, Markus Armbruster wrote:

John Snow  writes:


For the sake of keeping __init__ smaller (and treating it more like a
gallery of what state variables we can expect to see), put the actual
parsing action into a parse method. It remains invoked from the init
method to reduce churn.


I'm kind of *shrug* about this.  Well short of "no".



Understood. Hopefully it's lateral movement at best to you; I don't want 
to make it distinctly worse in your eyes.




To accomplish this, 'previously_included' becomes the private data
member '_included', and the filename is stashed as _fname.


Nitpick: you enclose most identifiers in quotes, but not all.

Comments and commit messages often profit from "marking up" identifiers.
Especially when the identifiers are also common words.  I like to use
function(), @variable, .attribute, and .method().



Consistency in this regard is ... OK, not my strong suite. It can be 
hard to remember when to use which systems, and I'm caught between md, 
rst, kerneldoc, qapidoc, etc.


More or less, I am always happy if you just edit little things like this 
on pull as you see fit, I won't be mad.


I'll edit this in my local branch for now.

--js



Add any missing declarations to the init method, and group them by
function so they can be understood quickly at a glance.

Signed-off-by: John Snow 

Re: [PATCH v2 04/21] qapi/parser: factor parsing routine into method

[Markus Armbruster]

John Snow  writes:

> For the sake of keeping __init__ smaller (and treating it more like a
> gallery of what state variables we can expect to see), put the actual
> parsing action into a parse method. It remains invoked from the init
> method to reduce churn.

I'm kind of *shrug* about this.  Well short of "no".

>
> To accomplish this, 'previously_included' becomes the private data
> member '_included', and the filename is stashed as _fname.

Nitpick: you enclose most identifiers in quotes, but not all.

Comments and commit messages often profit from "marking up" identifiers.
Especially when the identifiers are also common words.  I like to use
function(), @variable, .attribute, and .method().

>
> Add any missing declarations to the init method, and group them by
> function so they can be understood quickly at a glance.
>
> Signed-off-by: John Snow 

[PATCH v2 04/21] qapi/parser: factor parsing routine into method

[John Snow]

For the sake of keeping __init__ smaller (and treating it more like a
gallery of what state variables we can expect to see), put the actual
parsing action into a parse method. It remains invoked from the init
method to reduce churn.

To accomplish this, 'previously_included' becomes the private data
member '_included', and the filename is stashed as _fname.

Add any missing declarations to the init method, and group them by
function so they can be understood quickly at a glance.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 40 
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 39dbcc4eacc..d620706fffb 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -37,23 +37,39 @@ def __init__(self, parser, msg):
 class QAPISchemaParser:
 
 def __init__(self, fname, previously_included=None, incl_info=None):
-previously_included = previously_included or set()
-previously_included.add(os.path.abspath(fname))
+self._fname = fname
+self._included = previously_included or set()
+self._included.add(os.path.abspath(self._fname))
+self.src = ''
 
-# May raise OSError; allow the caller to handle it.
-with open(fname, 'r', encoding='utf-8') as fp:
-self.src = fp.read()
-
-if self.src == '' or self.src[-1] != '\n':
-self.src += '\n'
+# Lexer state (see `accept` for details):
+self.info = QAPISourceInfo(self._fname, incl_info)
+self.tok = None
+self.pos = 0
 self.cursor = 0
-self.info = QAPISourceInfo(fname, incl_info)
+self.val = None
 self.line_pos = 0
+
+# Parser output:
 self.exprs = []
 self.docs = []
-self.accept()
+
+# Showtime!
+self._parse()
+
+def _parse(self):
 cur_doc = None
 
+# May raise OSError; allow the caller to handle it.
+with open(self._fname, 'r', encoding='utf-8') as fp:
+self.src = fp.read()
+if self.src == '' or self.src[-1] != '\n':
+self.src += '\n'
+
+# Prime the lexer:
+self.accept()
+
+# Parse until done:
 while self.tok is not None:
 info = self.info
 if self.tok == '#':
@@ -71,12 +87,12 @@ def __init__(self, fname, previously_included=None, 
incl_info=None):
 if not isinstance(include, str):
 raise QAPISemError(info,
"value of 'include' must be a string")
-incl_fname = os.path.join(os.path.dirname(fname),
+incl_fname = os.path.join(os.path.dirname(self._fname),
   include)
 self.exprs.append({'expr': {'include': incl_fname},
'info': info})
 exprs_include = self._include(include, info, incl_fname,
-  previously_included)
+  self._included)
 if exprs_include:
 self.exprs.extend(exprs_include.exprs)
 self.docs.extend(exprs_include.docs)
-- 
2.30.2