On 1/13/21 11:12 AM, Markus Armbruster wrote:
John Snow <js...@redhat.com> writes:
Signed-off-by: John Snow <js...@redhat.com>
---
The event_enum_members change might become irrelevant after a
forthcoming (?) patch by Markus, but didn't have it in-hand at time of
publishing.
It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
which includes parts of your v1. The parts that are new should be
injected into your series so they replace your "[PATCH v2 09/12]
qapi/gen: move write method to QAPIGenC, make fname a str". Holler if
you need help.
Signed-off-by: John Snow <js...@redhat.com>
---
scripts/qapi/events.py | 2 +-
scripts/qapi/schema.py | 25 ++++++++++++++-----------
scripts/qapi/types.py | 9 +++++----
scripts/qapi/visit.py | 6 +++---
4 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d1..9ba4f109028 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -225,7 +225,7 @@ def visit_event(self,
self._event_emit_name))
# Note: we generate the enum member regardless of @ifcond, to
# keep the enumeration usable in target-independent code.
- self._event_enum_members.append(QAPISchemaEnumMember(name, None))
+ self._event_enum_members.append(QAPISchemaEnumMember(name, info))
This "enum" is not supposed to be erroneous. If it is, it's a bug.
Your patch changes how the code behaves should such a bug bite here.
Before, we crash. Afterwards, we report the bug using @info, which I'd
expect to produce utterly confusing error messages.
It doesn't change the behavior *here*, though. It changes it whenever
this info object is used in another context. ... and who knows when or
where or why it is being used, or by whom.
I'll have to play with this. I'm not sure there's any way to coax a bug
to happen here that I am aware of right away. Can you think of how to
will one into existence?
My comments on PATCH 06 apply: how the code should behave here is a
design issue that should be kept out of this patch series.
If you need to pass a value other than None to help with static typing,
then pass a suitable poison info that will crash right where None
crashes now.
I think we need to, yes; or we probably really, really want to. Making
the info parameter optional really adds a lot of unidiomatic
type-checking confetti when we go to use info, and in many more contexts
than just this sorta-built-in-enum; it will creep badly.
So, I gotta pass ...something here. but what? You want poison, but I
think it's not right to fundamentally poison all built-ins.
Mmmmmmm. Maybe per-instance poison can be done? We actually share info
objects, but I can make poisoned copies. Using next_line() as a basis:
def poison(self: T) -> T:
info = copy.copy(self)
info.poisoned = True
return info
probably almost anything I do is not going to make a lot of sense unless
I can actually replicate and test the different error scenarios to prove
that we didn't make the error spaghetti unknowably worse. I see it as
functionally inevitable that I have to audit this and make sure we get
good error messages anyway, so ... maybe I just ought to do that now anyway.
def gen_events(schema: QAPISchema,
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..0449771dfe5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -23,6 +23,7 @@
from .error import QAPIError, QAPISemError
from .expr import check_exprs
from .parser import QAPISchemaParser
+from .source import QAPISourceInfo
class QAPISchemaEntity:
@@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None,
features=None):
self.name = name
self._module = None
# For explicitly defined entities, info points to the (explicit)
- # definition. For builtins (and their arrays), info is None.
- # For implicitly defined entities, info points to a place that
- # triggered the implicit definition (there may be more than one
- # such place).
+ # definition. For built-in types (and their arrays), info is a
+ # special object that evaluates to False. For implicitly defined
+ # entities, info points to a place that triggered the implicit
+ # definition (there may be more than one such place).
self.info = info
self.doc = doc
self._ifcond = ifcond or []
@@ -68,7 +69,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
- self._module = schema.module_by_fname(info and info.fname)
+ self._module = schema.module_by_fname(info.fname if info else None)
Looks unrelated.
Hmm, it sorta is. I have apparently edited this patch since I sent it,
but there was some tomfoolery over how "x and y" statements behave and
this edit was necessary at the time.
"info and info.fname" returned None when info could actually be None,
but when it was updated to be a special source object, we could
accidentally pass that special source object as the name -- instead of
None. Not good.
I think I re-ordered some patches such that I can just pass in
"info.fname" unconditionally instead as of this patch.
self._module.add_entity(self)
def set_module(self, schema):
[...]
