qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH v3 3/9] qapi: start building an 'if' predicate tree


From: John Snow
Subject: Re: [PATCH v3 3/9] qapi: start building an 'if' predicate tree
Date: Fri, 21 May 2021 12:18:03 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

On 5/21/21 10:48 AM, Markus Armbruster wrote:
Beware, I'm skimming, not really reviewing.

John Snow <jsnow@redhat.com> writes:

On 4/29/21 9:40 AM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
The following patches are going to express schema 'if' conditions in
a
target language agnostic way. For that, let's start building a predicate
tree of the configuration options.
This intermediary steps still uses C-preprocessor expressions as
the predicates:
"if: [STR, ..]" is translated to a "IfCond -> IfAll ->
[IfOption, ..]" tree, which will generate "#if STR && .." C code.
Once the boolean operation tree nodes are introduced, the 'if'
expressions will be converted to replace the C syntax (no more
!defined(), &&, ...) and based only on option identifiers.
For now, the condition tree will be less expressive than a full C
macro
expression as it will only support these operations: 'all', 'any' and
'not', the only ones needed so far.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
   docs/sphinx/qapidoc.py                 |  6 +--
   scripts/qapi/common.py                 | 54 +++++++++++++++++++++++-
   scripts/qapi/schema.py                 | 42 ++++++++++++-------
   tests/qapi-schema/doc-good.out         | 12 +++---
   tests/qapi-schema/qapi-schema-test.out | 58 +++++++++++++-------------
   5 files changed, 116 insertions(+), 56 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index b737949007..a93f3f1c4d 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -112,12 +112,10 @@ def _make_section(self, title):
       def _nodes_for_ifcond(self, ifcond, with_if=True):
           """Return list of Text, literal nodes for the ifcond
   -        Return a list which gives text like ' (If: cond1, cond2,
cond3)', where
-        the conditions are in literal-text and the commas are not.
+        Return a list which gives text like ' (If: condition)'.
           If with_if is False, we don't return the "(If: " and ")".
           """
-        condlist = intersperse([nodes.literal('', c) for c in ifcond.ifcond],
-                               nodes.Text(', '))

was this the last user of intersperse?

mm, no, there's one more.

+        condlist = [nodes.literal('', ifcond.gen_doc())]
           if not with_if:
               return condlist
   diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index b7f475a160..59a7ee2f32 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -11,8 +11,9 @@
   # This work is licensed under the terms of the GNU GPL, version 2.
   # See the COPYING file in the top-level directory.
   +from abc import ABC, abstractmethod
   import re
-from typing import Optional
+from typing import Optional, Sequence
#: Magic string that gets removed along with all space to its right.
@@ -192,3 +193,54 @@ def guardend(name: str) -> str:
   #endif /* %(name)s */
   ''',
                    name=c_fname(name).upper())
+
+
+class IfPredicate(ABC):
+    @abstractmethod
+    def cgen(self) -> str:

Like the review to patch 2, I'm not sure we want to bake cgen stuff
directly into this class. Are you going to have cgen() and rustgen()
methods all here?

+        pass
+

I think you want raise NotImplementedError to specify a function that
the inheriting class MUST implement. Otherwise, there's little value
to allow a child class to call super() on a method that doesn't have a
default implementation.

You *could* drop the (ABC) and the @abstractmethod decorators if you do so.

Matters of taste and style.

We're not coding a library for use by thousands of people.  If we did,
then complicating the code to guard against misuse could be a win.  But
we don't.

schema.py is full of methods that pass.  Maybe some of them need to be
overriden by any conceivable subtype.  Who cares?  The subtypes either
work or they don't.  I prefer

     def frobnicate:
         pass

to

     def frobnicate:
         raise NotImplementedError

One, pass is easier on the eyes.  Two, a subtype's .frobnicate() can
blindly call super().frobnicate().


"pass" here operates as a syntactic sugar for "return None" which has implications on the return type. It's short, but wrong.

raise NotImplementedError means something different semantically.

To me, pass is *NOT* easier on the eyes, it is misleading. It is idiomatic to use NotImplementedError if it is not acceptable for a default implementation to return None.

I understand perfectly well the desire to keep things simple, but what is actually "simple" depends on the expectations of the programmer. I err towards idiomatic Python.

I don't see a need to fuzz around with module abc, either.  Let's stick
to the stupidest solution that could possibly work.

[...]





reply via email to

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