| Message ID | 20200915224027.2529813-11-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
John Snow <jsnow@redhat.com> writes: > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/common.py | 16 +++++++--------- > scripts/qapi/schema.py | 14 +++++++------- > 2 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index 87d87b95e5..c665e67495 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -14,6 +14,11 @@ > import re > > > +EATSPACE = '\033EATSPACE.' > +POINTER_SUFFIX = ' *' + EATSPACE > +c_name_trans = str.maketrans('.-', '__') > + > + You rename and move. pylint gripes about the names, but it doesn't actually ask for the move, as far as I can tell. Can you explain why you move? > # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 > # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 > # ENUM24_Name -> ENUM24_NAME > @@ -42,9 +47,6 @@ def c_enum_const(type_name, const_name, prefix=None): > return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() > > > -c_name_trans = str.maketrans('.-', '__') > - > - > # Map @name to a valid C identifier. > # If @protect, avoid returning certain ticklish identifiers (like > # C keywords) by prepending 'q_'. > @@ -89,10 +91,6 @@ def c_name(name, protect=True): > return name > > > -eatspace = '\033EATSPACE.' > -pointer_suffix = ' *' + eatspace > - > - > class Indent: > """ > Indent-level management. > @@ -135,12 +133,12 @@ def pop(self, amount: int = 4) -> int: > > > # Generate @code with @kwds interpolated. > -# Obey indent_level, and strip eatspace. > +# Obey INDENT level, and strip EATSPACE. Is the change to INDENT intentional? > def cgen(code, **kwds): > raw = code % kwds > if INDENT: > raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE) > - return re.sub(re.escape(eatspace) + r' *', '', raw) > + return re.sub(re.escape(EATSPACE) + r' *', '', raw) > > > def mcgen(code, **kwds): [...]
On 9/17/20 10:15 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/common.py | 16 +++++++--------- >> scripts/qapi/schema.py | 14 +++++++------- >> 2 files changed, 14 insertions(+), 16 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index 87d87b95e5..c665e67495 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -14,6 +14,11 @@ >> import re >> >> >> +EATSPACE = '\033EATSPACE.' >> +POINTER_SUFFIX = ' *' + EATSPACE >> +c_name_trans = str.maketrans('.-', '__') >> + >> + > > You rename and move. pylint gripes about the names, but it doesn't > actually ask for the move, as far as I can tell. Can you explain why > you move? > Preference. I like constants and globals at the top so you can audit any code that runs at import time in one place. Since they are externally visible objects, having them near other "header" style information makes sense to me. >> # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 >> # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 >> # ENUM24_Name -> ENUM24_NAME >> @@ -42,9 +47,6 @@ def c_enum_const(type_name, const_name, prefix=None): >> return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() >> >> >> -c_name_trans = str.maketrans('.-', '__') (This one winds up being a constant, so I renamed it in my v2.) >> - >> - >> # Map @name to a valid C identifier. >> # If @protect, avoid returning certain ticklish identifiers (like >> # C keywords) by prepending 'q_'. >> @@ -89,10 +91,6 @@ def c_name(name, protect=True): >> return name >> >> >> -eatspace = '\033EATSPACE.' >> -pointer_suffix = ' *' + eatspace >> - >> - >> class Indent: >> """ >> Indent-level management. >> @@ -135,12 +133,12 @@ def pop(self, amount: int = 4) -> int: >> >> >> # Generate @code with @kwds interpolated. >> -# Obey indent_level, and strip eatspace. >> +# Obey INDENT level, and strip EATSPACE. > > Is the change to INDENT intentional? > Kind of, but it's either late (should have been with the indent manager patch) or early (Should be with the patch that moves comments into docstrings.) When this comment becomes a docstring, I use `INDENT` to indicate it as a proper object. This in and of itself is prescient, as we are not using sphinx/apidoc to generate any documentation about the QAPI package yet. (The pending v2 uses `indent` after you pointed out that it was not a constant.) >> def cgen(code, **kwds): >> raw = code % kwds >> if INDENT: >> raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE) >> - return re.sub(re.escape(eatspace) + r' *', '', raw) >> + return re.sub(re.escape(EATSPACE) + r' *', '', raw) >> >> >> def mcgen(code, **kwds): > [...] >
John Snow <jsnow@redhat.com> writes: > On 9/17/20 10:15 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/common.py | 16 +++++++--------- >>> scripts/qapi/schema.py | 14 +++++++------- >>> 2 files changed, 14 insertions(+), 16 deletions(-) >>> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index 87d87b95e5..c665e67495 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -14,6 +14,11 @@ >>> import re >>> >>> +EATSPACE = '\033EATSPACE.' >>> +POINTER_SUFFIX = ' *' + EATSPACE >>> +c_name_trans = str.maketrans('.-', '__') >>> + >>> + >> You rename and move. pylint gripes about the names, but it doesn't >> actually ask for the move, as far as I can tell. Can you explain why >> you move? >> > > Preference. I like constants and globals at the top so you can audit > any code that runs at import time in one place. I can buy this argument. > Since they are > externally visible objects, having them near other "header" style > information makes sense to me. This one I find unconvincing. Functions and classes are just as visible. Mention the move in the commit message, along with the (convincing part of the) rationale. Aside: EATSPACE and c_name_trans are actually local to common.py. EATSPACE sort of leaks out only via the contract of mcgen(). The contract could be rephrased in terms of POINTER_SUFFIX. Not sure it's worthwhile. >>> # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 >>> # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 >>> # ENUM24_Name -> ENUM24_NAME >>> @@ -42,9 +47,6 @@ def c_enum_const(type_name, const_name, prefix=None): >>> return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() >>> >>> -c_name_trans = str.maketrans('.-', '__') > > (This one winds up being a constant, so I renamed it in my v2.) > >>> - >>> - >>> # Map @name to a valid C identifier. >>> # If @protect, avoid returning certain ticklish identifiers (like >>> # C keywords) by prepending 'q_'. >>> @@ -89,10 +91,6 @@ def c_name(name, protect=True): >>> return name >>> >>> -eatspace = '\033EATSPACE.' >>> -pointer_suffix = ' *' + eatspace >>> - >>> - >>> class Indent: >>> """ >>> Indent-level management. >>> @@ -135,12 +133,12 @@ def pop(self, amount: int = 4) -> int: >>> >>> # Generate @code with @kwds interpolated. >>> -# Obey indent_level, and strip eatspace. >>> +# Obey INDENT level, and strip EATSPACE. >> Is the change to INDENT intentional? >> > > Kind of, but it's either late (should have been with the indent > manager patch) or early (Should be with the patch that moves comments > into docstrings.) > > When this comment becomes a docstring, I use `INDENT` to indicate it > as a proper object. This in and of itself is prescient, as we are not > using sphinx/apidoc to generate any documentation about the QAPI > package yet. > > (The pending v2 uses `indent` after you pointed out that it was not a > constant.) > >>> def cgen(code, **kwds): >>> raw = code % kwds >>> if INDENT: >>> raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE) >>> - return re.sub(re.escape(eatspace) + r' *', '', raw) >>> + return re.sub(re.escape(EATSPACE) + r' *', '', raw) >>> >>> def mcgen(code, **kwds): >> [...] >>
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 87d87b95e5..c665e67495 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -14,6 +14,11 @@ import re +EATSPACE = '\033EATSPACE.' +POINTER_SUFFIX = ' *' + EATSPACE +c_name_trans = str.maketrans('.-', '__') + + # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1 # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2 # ENUM24_Name -> ENUM24_NAME @@ -42,9 +47,6 @@ def c_enum_const(type_name, const_name, prefix=None): return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() -c_name_trans = str.maketrans('.-', '__') - - # Map @name to a valid C identifier. # If @protect, avoid returning certain ticklish identifiers (like # C keywords) by prepending 'q_'. @@ -89,10 +91,6 @@ def c_name(name, protect=True): return name -eatspace = '\033EATSPACE.' -pointer_suffix = ' *' + eatspace - - class Indent: """ Indent-level management. @@ -135,12 +133,12 @@ def pop(self, amount: int = 4) -> int: # Generate @code with @kwds interpolated. -# Obey indent_level, and strip eatspace. +# Obey INDENT level, and strip EATSPACE. def cgen(code, **kwds): raw = code % kwds if INDENT: raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE) - return re.sub(re.escape(eatspace) + r' *', '', raw) + return re.sub(re.escape(EATSPACE) + r' *', '', raw) def mcgen(code, **kwds): diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index b77e0e56b2..b4921b46c9 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -18,7 +18,7 @@ import re from collections import OrderedDict -from .common import c_name, pointer_suffix +from .common import c_name, POINTER_SUFFIX from .error import QAPIError, QAPISemError from .expr import check_exprs from .parser import QAPISchemaParser @@ -309,7 +309,7 @@ def is_implicit(self): return True def c_type(self): - return c_name(self.name) + pointer_suffix + return c_name(self.name) + POINTER_SUFFIX def json_type(self): return 'array' @@ -430,7 +430,7 @@ def c_name(self): def c_type(self): assert not self.is_implicit() - return c_name(self.name) + pointer_suffix + return c_name(self.name) + POINTER_SUFFIX def c_unboxed_type(self): return c_name(self.name) @@ -504,7 +504,7 @@ def connect_doc(self, doc=None): v.connect_doc(doc) def c_type(self): - return c_name(self.name) + pointer_suffix + return c_name(self.name) + POINTER_SUFFIX def json_type(self): return 'value' @@ -896,7 +896,7 @@ def _def_builtin_type(self, name, json_type, c_type): self._make_array_type(name, None) def _def_predefineds(self): - for t in [('str', 'string', 'char' + pointer_suffix), + for t in [('str', 'string', 'char' + POINTER_SUFFIX), ('number', 'number', 'double'), ('int', 'int', 'int64_t'), ('int8', 'int', 'int8_t'), @@ -909,8 +909,8 @@ def _def_predefineds(self): ('uint64', 'int', 'uint64_t'), ('size', 'int', 'uint64_t'), ('bool', 'boolean', 'bool'), - ('any', 'value', 'QObject' + pointer_suffix), - ('null', 'null', 'QNull' + pointer_suffix)]: + ('any', 'value', 'QObject' + POINTER_SUFFIX), + ('null', 'null', 'QNull' + POINTER_SUFFIX)]: self._def_builtin_type(*t) self.the_empty_object_type = QAPISchemaObjectType( 'q_empty', None, None, None, None, None, [], None)
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/common.py | 16 +++++++--------- scripts/qapi/schema.py | 14 +++++++------- 2 files changed, 14 insertions(+), 16 deletions(-)