Marc-André Lureau <marcandre.lur...@redhat.com> writes:
> Hi
>
> On Mon, Aug 2, 2021 at 1:21 PM Markus Armbruster <arm...@redhat.com> wrote:
>
>> marcandre.lur...@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
>> >
>> > Except for the special casing assert in _make_implicit_object_type(),
>> > which needs to handle schema objects, it's a mechanical change.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> > ---
[...]
>> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> > index 9a348ca2e5..77a8c33ad4 100644
>> > --- a/scripts/qapi/introspect.py
>> > +++ b/scripts/qapi/introspect.py
[...]
>> > @@ -125,10 +124,10 @@ def indent(level: int) -> str:
>> > if obj.comment:
>> > ret += indent(level) + f"/* {obj.comment} */\n"
>> > if obj.ifcond:
>> > - ret += gen_if(obj.ifcond)
>> > + ret += gen_if(obj.ifcond.ifcond)
>> > ret += _tree_to_qlit(obj.value, level)
>> > if obj.ifcond:
>> > - ret += '\n' + gen_endif(obj.ifcond)
>> > + ret += '\n' + gen_endif(obj.ifcond.ifcond)
>> > return ret
>> >
>> > ret = ''
>>
>> You update obj.ifcond to obj.ifcond.ifcond when used as argument of
>> gen_if() and gen_endif(). This changes the argument from Tuple to
>> Sequence. Fine, because Tuple is a special Sequence.
>>
>> Digression: I don't (anymore) understand why we made self.ifcond Tuple.
>> John, do you remember?
>>
>> You don't update obj.ifcond when used as conditional. The code now
>> calls gen_if() and gen_endif() even for an empty Sequence.
>>
>> I believe this can't actually happen because check_if() rejects [] with
>> "'if' condition [] of %s is useless".
>>
>> Still, the mechanical change should update to obj.ifcond even when used
>> as conditional.
>>
>> Are there other, possibly not so harmless uses of values that change
>> from Sequence to QAPISchemaIfCond the patch doesn't update?
>>
>> Or asked differently: how did you find what to update?
>>
>
> Eh, you are asking me for something I spent just a few hours a few times
> over the last year. Sorry!
That's fair.
> Most probably simply with code reading/grepping, linter and the test suite.
Plausible.
I'm somewhat worried about the bug pattern, but a thorough search feels
uneconomical. Instead, I've attempted two things to flush out such
bugs:
* Run "make check" with the appended patch. Fails with v6, i.e. it can
detect execution of the bug pattern. Passes with v7.
* The variables holding QAPISchemaIfCond are generally named @ifcond.
Eyeball grep -w ifcond with ifcond.MUMBLE, ifcond: QAPISchemaIfCond,
and def parameter lists ignored. Most remaining grep hits move values
around. I can see a few hits where @ifcond is used like a boolean:
- In docgen_ifcond() and cgen_ifcond(), but @ifcond isn't
QAPISchemaIfCond there. Okay, but there's an unrelated oddity I'll
discuss in review of PATCH 07.
- In QAPISchemaIfCond.__init__(): likewise.
- In QAPISchemaEntity.__init__() and QAPISchemaMember.__init__():
okay, because @ifcond is Optional[QAPISchemaIfCond] there.
- In check_if(), but @ifcond isn't QAPISchemaIfCond there. Okay.
I think that's all we can sensibly do now.
[...]
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 627735a431..a71ad31d59 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -43,6 +43,9 @@ def docgen(self):
def is_present(self):
return bool(self.ifcond)
+ def __bool__(self):
+ assert False
+
class QAPISchemaEntity:
meta: Optional[str] = None
@@ -61,7 +64,7 @@ def __init__(self, name: str, info, doc, ifcond=None,
features=None):
# such place).
self.info = info
self.doc = doc
- self._ifcond = ifcond or QAPISchemaIfCond()
+ self._ifcond = ifcond if ifcond != None else QAPISchemaIfCond()
self.features = features or []
self._checked = False
@@ -665,7 +668,7 @@ def __init__(self, name, info, ifcond=None):
assert isinstance(name, str)
self.name = name
self.info = info
- self.ifcond = ifcond or QAPISchemaIfCond()
+ self.ifcond = ifcond if ifcond != None else QAPISchemaIfCond()
self.defined_in = None
def set_defined_in(self, name):
