| Message ID | 20200922210101.4081073-11-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
On Tue, Sep 22, 2020 at 05:00:33PM -0400, John Snow wrote: > At this point, that just means using a consistent strategy for constant names. > constants get UPPER_CASE and names not used externally get a leading underscore. > > As a preference, while renaming constants to be UPPERCASE, move them to > the head of the file. Generally, it's nice to be able to audit the code > that runs on import in one central place. > > Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> -- Eduardo
On Tue, Sep 22, 2020 at 05:00:33PM -0400, John Snow wrote: > At this point, that just means using a consistent strategy for constant names. > constants get UPPER_CASE and names not used externally get a leading underscore. > > As a preference, while renaming constants to be UPPERCASE, move them to > the head of the file. Generally, it's nice to be able to audit the code > that runs on import in one central place. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/common.py | 18 ++++++++---------- > scripts/qapi/schema.py | 14 +++++++------- > 2 files changed, 15 insertions(+), 17 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index e0c5871b10..bddfb5a9e5 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('.-', '__') IMO _C_NAME_TRANS is solely the concern of the c_name() function, and should not be a global. If you're concerned with speed (which I don't think you should) you could still do: def c_name(name, protect=True, name_translation=str.maketrans('.-', '__')): ... name = name.translate(name_translation) Keeping in mind that you're adding a mutable type to a function argument *on purpose*. I'd really favor having that statement within the only function that uses it, though. - Cleber.
On 9/23/20 12:01 PM, Cleber Rosa wrote: > On Tue, Sep 22, 2020 at 05:00:33PM -0400, John Snow wrote: >> At this point, that just means using a consistent strategy for constant names. >> constants get UPPER_CASE and names not used externally get a leading underscore. >> >> As a preference, while renaming constants to be UPPERCASE, move them to >> the head of the file. Generally, it's nice to be able to audit the code >> that runs on import in one central place. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/common.py | 18 ++++++++---------- >> scripts/qapi/schema.py | 14 +++++++------- >> 2 files changed, 15 insertions(+), 17 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index e0c5871b10..bddfb5a9e5 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('.-', '__') > > IMO _C_NAME_TRANS is solely the concern of the c_name() function, and > should not be a global. If you're concerned with speed (which I don't > think you should) you could still do: > It's true, but that's why it's marked internal here with the leading underscore -- it will not be exported. It was also *already* defined at the module level, I didn't hoist it. I think what is written here is already the simplest thing that works. > def c_name(name, protect=True, > name_translation=str.maketrans('.-', '__')): > ... > name = name.translate(name_translation) > > Keeping in mind that you're adding a mutable type to a function > argument *on purpose*. I'd really favor having that statement within > the only function that uses it, though. > That complicates the signature, so I think we shouldn't. --js
On Wed, Sep 23, 2020 at 01:37:06PM -0400, John Snow wrote: > On 9/23/20 12:01 PM, Cleber Rosa wrote: > > On Tue, Sep 22, 2020 at 05:00:33PM -0400, John Snow wrote: > > > At this point, that just means using a consistent strategy for constant names. > > > constants get UPPER_CASE and names not used externally get a leading underscore. > > > > > > As a preference, while renaming constants to be UPPERCASE, move them to > > > the head of the file. Generally, it's nice to be able to audit the code > > > that runs on import in one central place. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > --- > > > scripts/qapi/common.py | 18 ++++++++---------- > > > scripts/qapi/schema.py | 14 +++++++------- > > > 2 files changed, 15 insertions(+), 17 deletions(-) > > > > > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > > > index e0c5871b10..bddfb5a9e5 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('.-', '__') > > > > IMO _C_NAME_TRANS is solely the concern of the c_name() function, and > > should not be a global. If you're concerned with speed (which I don't > > think you should) you could still do: > > > > It's true, but that's why it's marked internal here with the leading > underscore -- it will not be exported. It was also *already* defined at the > module level, I didn't hoist it. > > I think what is written here is already the simplest thing that works. > > > def c_name(name, protect=True, > > name_translation=str.maketrans('.-', '__')): > > ... > > name = name.translate(name_translation) > > > > Keeping in mind that you're adding a mutable type to a function > > argument *on purpose*. I'd really favor having that statement within > > the only function that uses it, though. > > > > That complicates the signature, so I think we shouldn't. > > --js > > Not a too strong opinion about this to block it. So if I hadn't already: Reviewed-by: Cleber Rosa <crosa@redhat.com>
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index e0c5871b10..bddfb5a9e5 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_'. @@ -82,17 +84,13 @@ def c_name(name, protect=True): 'not_eq', 'or', 'or_eq', 'xor', 'xor_eq']) # namespace pollution: polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386']) - name = name.translate(c_name_trans) + name = name.translate(_C_NAME_TRANS) if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words): return 'q_' + name return name -eatspace = '\033EATSPACE.' -pointer_suffix = ' *' + eatspace - - class Indentation: """ Indentation level management. @@ -134,12 +132,12 @@ def decrease(self, amount: int = 4) -> int: # Generate @code with @kwds interpolated. -# Obey indent, and strip eatspace. +# Obey indent, and strip EATSPACE. def cgen(code, **kwds): raw = code % kwds if indent: raw = re.sub(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)
At this point, that just means using a consistent strategy for constant names. constants get UPPER_CASE and names not used externally get a leading underscore. As a preference, while renaming constants to be UPPERCASE, move them to the head of the file. Generally, it's nice to be able to audit the code that runs on import in one central place. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/common.py | 18 ++++++++---------- scripts/qapi/schema.py | 14 +++++++------- 2 files changed, 15 insertions(+), 17 deletions(-)