qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 09/38] qapi/common.py: Add indent manager


From: John Snow
Subject: Re: [PATCH v2 09/38] qapi/common.py: Add indent manager
Date: Fri, 25 Sep 2020 10:42:54 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]