| Message ID | 20200922210101.4081073-10-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
On Tue, Sep 22, 2020 at 05:00:32PM -0400, John Snow wrote: > Code style tools really dislike the use of global keywords, because it > generally involves re-binding the name at runtime which can have strange > effects depending on when and how that global name is referenced in > other modules. > > Make a little indent level manager instead. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/common.py | 51 +++++++++++++++++++++++++++++------------- > scripts/qapi/visit.py | 7 +++--- > 2 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index cee63eb95c..e0c5871b10 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -93,33 +93,52 @@ def c_name(name, protect=True): > pointer_suffix = ' *' + eatspace > > > -def genindent(count): > - ret = '' > - for _ in range(count): > - ret += ' ' > - return ret > +class Indentation: > + """ > + Indentation level management. > > + :param initial: Initial number of spaces, default 0. > + """ > + def __init__(self, initial: int = 0) -> None: > + self._level = initial > > -indent_level = 0 > + def __int__(self) -> int: > + return self._level > > + def __repr__(self) -> str: > + return "{}({:d})".format(type(self).__name__, self._level) > > -def push_indent(indent_amount=4): > - global indent_level > - indent_level += indent_amount > + def __str__(self) -> str: > + """Return the current indentation as a string of spaces.""" > + return ' ' * self._level > > + def __bool__(self) -> bool: > + """True when there is a non-zero indentation.""" > + return bool(self._level) > > -def pop_indent(indent_amount=4): > - global indent_level > - indent_level -= indent_amount > + def increase(self, amount: int = 4) -> int: > + """Increase the indentation level by `amount`, default 4.""" > + self._level += amount > + return self._level > + > + def decrease(self, amount: int = 4) -> int: > + """Decrease the indentation level by `amount`, default 4.""" > + if self._level < amount: > + raise ArithmeticError( > + f"Can't remove {amount:d} spaces from {self!r}") > + self._level -= amount > + return self._level > + > + > +indent = Indentation() Personally, I would keep the push_indent(), pop_indent() API, and introduce an indent() function, to follow the existing API style of plain functions. Something like: indent_prefixes = [] def push_indent(amount=4): """Add `amount` spaces to indentation prefix""" indent_prefixes.push(' '*amount) def pop_indent(): """Revert the most recent push_indent() call""" indent_prefixes.pop() def indent(): """Return the current indentation prefix""" return ''.join(indent_prefixes) What you did is still an improvement, though, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> -- Eduardo
On Tue, Sep 22, 2020 at 05:00:32PM -0400, John Snow wrote: > Code style tools really dislike the use of global keywords, because it > generally involves re-binding the name at runtime which can have strange > effects depending on when and how that global name is referenced in > other modules. > > Make a little indent level manager instead. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/common.py | 51 +++++++++++++++++++++++++++++------------- > scripts/qapi/visit.py | 7 +++--- > 2 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index cee63eb95c..e0c5871b10 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -93,33 +93,52 @@ def c_name(name, protect=True): > pointer_suffix = ' *' + eatspace > > > -def genindent(count): > - ret = '' > - for _ in range(count): > - ret += ' ' > - return ret > +class Indentation: > + """ > + Indentation level management. > > + :param initial: Initial number of spaces, default 0. > + """ > + def __init__(self, initial: int = 0) -> None: > + self._level = initial > > -indent_level = 0 > + def __int__(self) -> int: > + return self._level > > + def __repr__(self) -> str: > + return "{}({:d})".format(type(self).__name__, self._level) > > -def push_indent(indent_amount=4): > - global indent_level > - indent_level += indent_amount > + def __str__(self) -> str: > + """Return the current indentation as a string of spaces.""" > + return ' ' * self._level > > + def __bool__(self) -> bool: > + """True when there is a non-zero indentation.""" > + return bool(self._level) > > -def pop_indent(indent_amount=4): > - global indent_level > - indent_level -= indent_amount > + def increase(self, amount: int = 4) -> int: > + """Increase the indentation level by `amount`, default 4.""" > + self._level += amount > + return self._level > + Do you have a use case for returning the level? If not, I'd go without it, and add a "level" property instead, as it'd serve more cases. Other than that, Reviewed-by: Cleber Rosa <crosa@redhat.com>
On 9/22/20 6:22 PM, Eduardo Habkost wrote: > On Tue, Sep 22, 2020 at 05:00:32PM -0400, John Snow wrote: >> Code style tools really dislike the use of global keywords, because it >> generally involves re-binding the name at runtime which can have strange >> effects depending on when and how that global name is referenced in >> other modules. >> >> Make a little indent level manager instead. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/common.py | 51 +++++++++++++++++++++++++++++------------- >> scripts/qapi/visit.py | 7 +++--- >> 2 files changed, 38 insertions(+), 20 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index cee63eb95c..e0c5871b10 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -93,33 +93,52 @@ def c_name(name, protect=True): >> pointer_suffix = ' *' + eatspace >> >> >> -def genindent(count): >> - ret = '' >> - for _ in range(count): >> - ret += ' ' >> - return ret >> +class Indentation: >> + """ >> + Indentation level management. >> >> + :param initial: Initial number of spaces, default 0. >> + """ >> + def __init__(self, initial: int = 0) -> None: >> + self._level = initial >> >> -indent_level = 0 >> + def __int__(self) -> int: >> + return self._level >> >> + def __repr__(self) -> str: >> + return "{}({:d})".format(type(self).__name__, self._level) >> >> -def push_indent(indent_amount=4): >> - global indent_level >> - indent_level += indent_amount >> + def __str__(self) -> str: >> + """Return the current indentation as a string of spaces.""" >> + return ' ' * self._level >> >> + def __bool__(self) -> bool: >> + """True when there is a non-zero indentation.""" >> + return bool(self._level) >> >> -def pop_indent(indent_amount=4): >> - global indent_level >> - indent_level -= indent_amount >> + def increase(self, amount: int = 4) -> int: >> + """Increase the indentation level by `amount`, default 4.""" >> + self._level += amount >> + return self._level >> + >> + def decrease(self, amount: int = 4) -> int: >> + """Decrease the indentation level by `amount`, default 4.""" >> + if self._level < amount: >> + raise ArithmeticError( >> + f"Can't remove {amount:d} spaces from {self!r}") >> + self._level -= amount >> + return self._level >> + >> + >> +indent = Indentation() > > Personally, I would keep the push_indent(), pop_indent() API, and > introduce an indent() function, to follow the existing API style > of plain functions. > > Something like: > > indent_prefixes = [] > def push_indent(amount=4): > """Add `amount` spaces to indentation prefix""" > indent_prefixes.push(' '*amount) > def pop_indent(): > """Revert the most recent push_indent() call""" > indent_prefixes.pop() > def indent(): > """Return the current indentation prefix""" > return ''.join(indent_prefixes) > > What you did is still an improvement, though, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Yeah, there's only one user right now, so ... I just kinda wanted to get rid of the global usage. Maybe if we make the code generator fancier we'll find out what form is best. --js
On 9/23/20 10:55 AM, Cleber Rosa wrote: > Do you have a use case for returning the level? If not, I'd go > without it, and add a "level" property instead, as it'd serve more > cases. __int__ is doing that lifting. I can remove the return. --js
John Snow <jsnow@redhat.com> writes: > On 9/22/20 6:22 PM, Eduardo Habkost wrote: >> On Tue, Sep 22, 2020 at 05:00:32PM -0400, John Snow wrote: >>> Code style tools really dislike the use of global keywords, because it >>> generally involves re-binding the name at runtime which can have strange >>> effects depending on when and how that global name is referenced in >>> other modules. >>> >>> Make a little indent level manager instead. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/common.py | 51 +++++++++++++++++++++++++++++------------- >>> scripts/qapi/visit.py | 7 +++--- >>> 2 files changed, 38 insertions(+), 20 deletions(-) >>> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index cee63eb95c..e0c5871b10 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -93,33 +93,52 @@ def c_name(name, protect=True): >>> pointer_suffix = ' *' + eatspace >>> >>> -def genindent(count): >>> - ret = '' >>> - for _ in range(count): >>> - ret += ' ' >>> - return ret >>> +class Indentation: >>> + """ >>> + Indentation level management. >>> + :param initial: Initial number of spaces, default 0. >>> + """ >>> + def __init__(self, initial: int = 0) -> None: >>> + self._level = initial >>> -indent_level = 0 >>> + def __int__(self) -> int: >>> + return self._level >>> + def __repr__(self) -> str: >>> + return "{}({:d})".format(type(self).__name__, self._level) >>> -def push_indent(indent_amount=4): >>> - global indent_level >>> - indent_level += indent_amount >>> + def __str__(self) -> str: >>> + """Return the current indentation as a string of spaces.""" >>> + return ' ' * self._level >>> + def __bool__(self) -> bool: >>> + """True when there is a non-zero indentation.""" >>> + return bool(self._level) >>> -def pop_indent(indent_amount=4): >>> - global indent_level >>> - indent_level -= indent_amount >>> + def increase(self, amount: int = 4) -> int: >>> + """Increase the indentation level by `amount`, default 4.""" >>> + self._level += amount >>> + return self._level >>> + >>> + def decrease(self, amount: int = 4) -> int: >>> + """Decrease the indentation level by `amount`, default 4.""" >>> + if self._level < amount: >>> + raise ArithmeticError( >>> + f"Can't remove {amount:d} spaces from {self!r}") >>> + self._level -= amount >>> + return self._level >>> + >>> + >>> +indent = Indentation() >> Personally, I would keep the push_indent(), pop_indent() API, and >> introduce an indent() function, to follow the existing API style >> of plain functions. >> Something like: >> indent_prefixes = [] >> def push_indent(amount=4): >> """Add `amount` spaces to indentation prefix""" >> indent_prefixes.push(' '*amount) >> def pop_indent(): >> """Revert the most recent push_indent() call""" >> indent_prefixes.pop() >> def indent(): >> """Return the current indentation prefix""" >> return ''.join(indent_prefixes) >> What you did is still an improvement, though, so: >> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> > > Yeah, there's only one user right now, so ... I just kinda wanted to > get rid of the global usage. Maybe if we make the code generator > fancier we'll find out what form is best. You don't get rid of the global variable, you just change it from integer to a class. A class can be handier when generating multiple things interleaved, because you can have one class instance per thing. Note that we already have a class instance per thing we generate: instances of subtypes of QAPIGen. The thought of moving the indentation machinery into QAPIGen or or maybe QAPIGenCCode crossed my mind many moons ago, but I had bigger fish to fry, and then I forgot :) John, I suggest you don't try to make this pretty just yet. Do what needs to be done for the type hint job. We can make it pretty later.
John Snow <jsnow@redhat.com> writes: > On 9/23/20 10:55 AM, Cleber Rosa wrote: >> Do you have a use case for returning the level? If not, I'd go >> without it, and add a "level" property instead, as it'd serve more >> cases. > > __int__ is doing that lifting. I can remove the return. I like my functions to return something useful. Use your judgement.
On Fri, Sep 25, 2020 at 01:51:54PM +0200, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > > On 9/22/20 6:22 PM, Eduardo Habkost wrote: [...] > > Yeah, there's only one user right now, so ... I just kinda wanted to > > get rid of the global usage. Maybe if we make the code generator > > fancier we'll find out what form is best. > > You don't get rid of the global variable, you just change it from > integer to a class. A class can be handier when generating multiple > things interleaved, because you can have one class instance per thing. To be fair, John doesn't claim to be getting rid of a global variable. He's getting rid of usage of the 'global' keyword to make linters happier. There are ways to do that without changing the code too much, though. > > Note that we already have a class instance per thing we generate: > instances of subtypes of QAPIGen. The thought of moving the indentation > machinery into QAPIGen or or maybe QAPIGenCCode crossed my mind many > moons ago, but I had bigger fish to fry, and then I forgot :) > > John, I suggest you don't try to make this pretty just yet. Do what > needs to be done for the type hint job. We can make it pretty later. -- Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Fri, Sep 25, 2020 at 01:51:54PM +0200, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> > On 9/22/20 6:22 PM, Eduardo Habkost wrote: > [...] >> > Yeah, there's only one user right now, so ... I just kinda wanted to >> > get rid of the global usage. Maybe if we make the code generator >> > fancier we'll find out what form is best. >> >> You don't get rid of the global variable, you just change it from >> integer to a class. A class can be handier when generating multiple >> things interleaved, because you can have one class instance per thing. > > To be fair, John doesn't claim to be getting rid of a global > variable. He's getting rid of usage of the 'global' keyword to > make linters happier. True. > There are ways to do that without changing > the code too much, though. Let's do something easy and cheap, because ... >> Note that we already have a class instance per thing we generate: >> instances of subtypes of QAPIGen. The thought of moving the indentation >> machinery into QAPIGen or or maybe QAPIGenCCode crossed my mind many >> moons ago, but I had bigger fish to fry, and then I forgot :) ... we'll probably want to move this stuff into QAPIGen later, and that's when we should make it pretty. >> John, I suggest you don't try to make this pretty just yet. Do what >> needs to be done for the type hint job. We can make it pretty later.
On 9/25/20 7:51 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 9/22/20 6:22 PM, Eduardo Habkost wrote: >>> On Tue, Sep 22, 2020 at 05:00:32PM -0400, John Snow wrote: >>>> Code style tools really dislike the use of global keywords, because it >>>> generally involves re-binding the name at runtime which can have strange >>>> effects depending on when and how that global name is referenced in >>>> other modules. >>>> >>>> Make a little indent level manager instead. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> scripts/qapi/common.py | 51 +++++++++++++++++++++++++++++------------- >>>> scripts/qapi/visit.py | 7 +++--- >>>> 2 files changed, 38 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>>> index cee63eb95c..e0c5871b10 100644 >>>> --- a/scripts/qapi/common.py >>>> +++ b/scripts/qapi/common.py >>>> @@ -93,33 +93,52 @@ def c_name(name, protect=True): >>>> pointer_suffix = ' *' + eatspace >>>> >>>> -def genindent(count): >>>> - ret = '' >>>> - for _ in range(count): >>>> - ret += ' ' >>>> - return ret >>>> +class Indentation: >>>> + """ >>>> + Indentation level management. >>>> + :param initial: Initial number of spaces, default 0. >>>> + """ >>>> + def __init__(self, initial: int = 0) -> None: >>>> + self._level = initial >>>> -indent_level = 0 >>>> + def __int__(self) -> int: >>>> + return self._level >>>> + def __repr__(self) -> str: >>>> + return "{}({:d})".format(type(self).__name__, self._level) >>>> -def push_indent(indent_amount=4): >>>> - global indent_level >>>> - indent_level += indent_amount >>>> + def __str__(self) -> str: >>>> + """Return the current indentation as a string of spaces.""" >>>> + return ' ' * self._level >>>> + def __bool__(self) -> bool: >>>> + """True when there is a non-zero indentation.""" >>>> + return bool(self._level) >>>> -def pop_indent(indent_amount=4): >>>> - global indent_level >>>> - indent_level -= indent_amount >>>> + def increase(self, amount: int = 4) -> int: >>>> + """Increase the indentation level by `amount`, default 4.""" >>>> + self._level += amount >>>> + return self._level >>>> + >>>> + def decrease(self, amount: int = 4) -> int: >>>> + """Decrease the indentation level by `amount`, default 4.""" >>>> + if self._level < amount: >>>> + raise ArithmeticError( >>>> + f"Can't remove {amount:d} spaces from {self!r}") >>>> + self._level -= amount >>>> + return self._level >>>> + >>>> + >>>> +indent = Indentation() >>> Personally, I would keep the push_indent(), pop_indent() API, and >>> introduce an indent() function, to follow the existing API style >>> of plain functions. >>> Something like: >>> indent_prefixes = [] >>> def push_indent(amount=4): >>> """Add `amount` spaces to indentation prefix""" >>> indent_prefixes.push(' '*amount) >>> def pop_indent(): >>> """Revert the most recent push_indent() call""" >>> indent_prefixes.pop() >>> def indent(): >>> """Return the current indentation prefix""" >>> return ''.join(indent_prefixes) >>> What you did is still an improvement, though, so: >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>> >> >> Yeah, there's only one user right now, so ... I just kinda wanted to >> get rid of the global usage. Maybe if we make the code generator >> fancier we'll find out what form is best. > > You don't get rid of the global variable, you just change it from > integer to a class. A class can be handier when generating multiple > things interleaved, because you can have one class instance per thing. > The 'global' keyword, not 'global scoped'. The key thing was to remove re-binding of the global name, which is now true. We bind to this object once and then modify object state. The global keyword allows us to re-bind the variable at the global scope which is the pattern to avoid. Having globally scoped things you don't rebind is perfectly fine. > Note that we already have a class instance per thing we generate: > instances of subtypes of QAPIGen. The thought of moving the indentation > machinery into QAPIGen or or maybe QAPIGenCCode crossed my mind many > moons ago, but I had bigger fish to fry, and then I forgot :) > > John, I suggest you don't try to make this pretty just yet. Do what > needs to be done for the type hint job. We can make it pretty later. > Mmhmm. I'm fine with with we have here for now. We can try to make things pretty later. We have the rust support series to consider, too, so I am not going to put the cart before the horse. --js
On 9/25/20 7:55 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 9/23/20 10:55 AM, Cleber Rosa wrote: >>> Do you have a use case for returning the level? If not, I'd go >>> without it, and add a "level" property instead, as it'd serve more >>> cases. >> >> __int__ is doing that lifting. I can remove the return. > > I like my functions to return something useful. > > Use your judgement. > > Eh, Cleber had a point. Nothing uses it. (AKA: I already made the edit ...) Like you say, we'll figure out the truly beautiful way to do code generation when we tackle this all together, holistically. What I've got works for now (and isn't terribly complex), let's just roll with it while we fry the bigger fish. --js
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index cee63eb95c..e0c5871b10 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -93,33 +93,52 @@ def c_name(name, protect=True): pointer_suffix = ' *' + eatspace -def genindent(count): - ret = '' - for _ in range(count): - ret += ' ' - return ret +class Indentation: + """ + Indentation level management. + :param initial: Initial number of spaces, default 0. + """ + def __init__(self, initial: int = 0) -> None: + self._level = initial -indent_level = 0 + def __int__(self) -> int: + return self._level + def __repr__(self) -> str: + return "{}({:d})".format(type(self).__name__, self._level) -def push_indent(indent_amount=4): - global indent_level - indent_level += indent_amount + def __str__(self) -> str: + """Return the current indentation as a string of spaces.""" + return ' ' * self._level + def __bool__(self) -> bool: + """True when there is a non-zero indentation.""" + return bool(self._level) -def pop_indent(indent_amount=4): - global indent_level - indent_level -= indent_amount + def increase(self, amount: int = 4) -> int: + """Increase the indentation level by `amount`, default 4.""" + self._level += amount + return self._level + + def decrease(self, amount: int = 4) -> int: + """Decrease the indentation level by `amount`, default 4.""" + if self._level < amount: + raise ArithmeticError( + f"Can't remove {amount:d} spaces from {self!r}") + self._level -= amount + return self._level + + +indent = Indentation() # Generate @code with @kwds interpolated. -# Obey indent_level, and strip eatspace. +# Obey indent, and strip eatspace. def cgen(code, **kwds): raw = code % kwds - if indent_level: - indent = genindent(indent_level) - raw = re.sub(r'^(?!(#|$))', indent, raw, flags=re.MULTILINE) + if indent: + raw = re.sub(r'^(?!(#|$))', str(indent), raw, flags=re.MULTILINE) return re.sub(re.escape(eatspace) + r' *', '', raw) diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 808410d6f1..4edaee33e3 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -19,8 +19,7 @@ gen_endif, gen_if, mcgen, - pop_indent, - push_indent, + indent, ) from .gen import QAPISchemaModularCVisitor, ifcontext from .schema import QAPISchemaObjectType @@ -69,7 +68,7 @@ def gen_visit_object_members(name, base, members, variants): if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) { ''', name=memb.name, c_name=c_name(memb.name)) - push_indent() + indent.increase() ret += mcgen(''' if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) { return false; @@ -78,7 +77,7 @@ def gen_visit_object_members(name, base, members, variants): c_type=memb.type.c_name(), name=memb.name, c_name=c_name(memb.name)) if memb.optional: - pop_indent() + indent.decrease() ret += mcgen(''' } ''')
Code style tools really dislike the use of global keywords, because it generally involves re-binding the name at runtime which can have strange effects depending on when and how that global name is referenced in other modules. Make a little indent level manager instead. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/common.py | 51 +++++++++++++++++++++++++++++------------- scripts/qapi/visit.py | 7 +++--- 2 files changed, 38 insertions(+), 20 deletions(-)