| Message ID | 20210608120723.2268181-4-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: untie 'if' conditions from C preprocessor | expand |
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Instead of building prepocessor conditions from a list of string, use > the result generated from QAPISchemaIfCond.cgen(). I understand why you're doing this, but only because I know where you're headed. By itself, it is not an improvement: you move C generation out of common.py into schema.py. You need to explain why that's useful. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > scripts/qapi/common.py | 24 +++++++++++------------- > scripts/qapi/gen.py | 4 ++-- > scripts/qapi/introspect.py | 4 ++-- > scripts/qapi/schema.py | 3 +++ > scripts/qapi/types.py | 20 ++++++++++---------- > scripts/qapi/visit.py | 12 ++++++------ > 6 files changed, 34 insertions(+), 33 deletions(-) Missing: qapi-code-gen.txt section "Configuring the schema" has an example, which needs to be updated. When the generated code changes, always check the examples, and always consider describing the change in the commit message. > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index 6ad1eeb61d..c305aaf2f1 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -12,7 +12,7 @@ > # See the COPYING file in the top-level directory. > > import re > -from typing import Match, Optional, Sequence > +from typing import Match, Optional > > > #: Magic string that gets removed along with all space to its right. > @@ -194,22 +194,20 @@ def guardend(name: str) -> str: > name=c_fname(name).upper()) > > > -def gen_if(ifcond: Sequence[str]) -> str: > - ret = '' > - for ifc in ifcond: > - ret += mcgen(''' > +def gen_if(cond: str) -> str: > + if not cond: > + return '' > + return mcgen(''' > #if %(cond)s > -''', cond=ifc) > - return ret > +''', cond=cond) > > > -def gen_endif(ifcond: Sequence[str]) -> str: > - ret = '' > - for ifc in reversed(ifcond): > - ret += mcgen(''' > +def gen_endif(cond: str) -> str: > + if not cond: > + return '' > + return mcgen(''' > #endif /* %(cond)s */ > -''', cond=ifc) > - return ret > +''', cond=cond) > > > def must_match(pattern: str, string: str) -> Match[str]: > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index 1c5b190276..51a597a025 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str: > if added[0] == '\n': > out += '\n' > added = added[1:] > - out += gen_if(ifcond.ifcond) > + out += gen_if(ifcond.cgen()) > out += added > - out += gen_endif(ifcond.ifcond) > + out += gen_endif(ifcond.cgen()) > return out > > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 77a8c33ad4..474b08fd4d 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -124,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.ifcond) > + ret += gen_if(obj.ifcond.cgen()) > ret += _tree_to_qlit(obj.value, level) > if obj.ifcond: > - ret += '\n' + gen_endif(obj.ifcond.ifcond) > + ret += '\n' + gen_endif(obj.ifcond.cgen()) > return ret > > ret = '' > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index bc357ebbfa..aa4715c519 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -29,6 +29,9 @@ class QAPISchemaIfCond: > def __init__(self, ifcond=None): > self.ifcond = ifcond or [] > > + def cgen(self): > + return ' && '.join(self.ifcond) Fragile. Better: return '(' + ') && ('.join(self.ifcond) + ')' If we want to keep C generation details out of schema.py, we need a helper mapping self.ifcond: Sequence[str] to C code, similar to how QAPISchemaEntity.c_name() works with helper c_name(). > + > # Returns true if the condition is not void > def __bool__(self): > return bool(self.ifcond) > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 3673cf0f49..db9ff95bd1 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -51,13 +51,13 @@ def gen_enum_lookup(name: str, > ''', > c_name=c_name(name)) > for memb in members: > - ret += gen_if(memb.ifcond.ifcond) > + ret += gen_if(memb.ifcond.cgen()) > index = c_enum_const(name, memb.name, prefix) > ret += mcgen(''' > [%(index)s] = "%(name)s", > ''', > index=index, name=memb.name) > - ret += gen_endif(memb.ifcond.ifcond) > + ret += gen_endif(memb.ifcond.cgen()) > > ret += mcgen(''' > }, [More of the same snipped...]
Markus Armbruster <armbru@redhat.com> writes: > marcandre.lureau@redhat.com writes: > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> Instead of building prepocessor conditions from a list of string, use >> the result generated from QAPISchemaIfCond.cgen(). > > I understand why you're doing this, but only because I know where you're > headed. By itself, it is not an improvement: you move C generation out > of common.py into schema.py. You need to explain why that's useful. > >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> scripts/qapi/common.py | 24 +++++++++++------------- >> scripts/qapi/gen.py | 4 ++-- >> scripts/qapi/introspect.py | 4 ++-- >> scripts/qapi/schema.py | 3 +++ >> scripts/qapi/types.py | 20 ++++++++++---------- >> scripts/qapi/visit.py | 12 ++++++------ >> 6 files changed, 34 insertions(+), 33 deletions(-) > > Missing: qapi-code-gen.txt section "Configuring the schema" has an > example, which needs to be updated. Nope, that's in PATCH 1 already. > When the generated code changes, always check the examples, and always > consider describing the change in the commit message. Describing the change in the commit message is even more useful when the doc update isn't in the same patch. [...]
Hi On Mon, Jun 14, 2021 at 4:48 PM Markus Armbruster <armbru@redhat.com> wrote: > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Instead of building prepocessor conditions from a list of string, use > > the result generated from QAPISchemaIfCond.cgen(). > > I understand why you're doing this, but only because I know where you're > headed. By itself, it is not an improvement: you move C generation out > of common.py into schema.py. You need to explain why that's useful. > > What about? In the following commits, QAPISchemaIfCond is going to hold an internal tree structure. Moving cgen() there allows to abstract away the condition representation. > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index bc357ebbfa..aa4715c519 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -29,6 +29,9 @@ class QAPISchemaIfCond: > > def __init__(self, ifcond=None): > > self.ifcond = ifcond or [] > > > > + def cgen(self): > > + return ' && '.join(self.ifcond) > > Fragile. Better: > > return '(' + ') && ('.join(self.ifcond) + ')' > > This is an intermediary step, but ok. If we want to keep C generation details out of schema.py, we need a > helper mapping self.ifcond: Sequence[str] to C code, similar to how > QAPISchemaEntity.c_name() works with helper c_name(). > Leaving a FIXME. > > + > > # Returns true if the condition is not void > > def __bool__(self): > > return bool(self.ifcond) > > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > > index 3673cf0f49..db9ff95bd1 100644 > > --- a/scripts/qapi/types.py > > +++ b/scripts/qapi/types.py > > @@ -51,13 +51,13 @@ def gen_enum_lookup(name: str, > > ''', > > c_name=c_name(name)) > > for memb in members: > > - ret += gen_if(memb.ifcond.ifcond) > > + ret += gen_if(memb.ifcond.cgen()) > > index = c_enum_const(name, memb.name, prefix) > > ret += mcgen(''' > > [%(index)s] = "%(name)s", > > ''', > > index=index, name=memb.name) > > - ret += gen_endif(memb.ifcond.ifcond) > > + ret += gen_endif(memb.ifcond.cgen()) > > > > ret += mcgen(''' > > }, > [More of the same snipped...] > > >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Jun 14, 2021 at 4:48 PM Markus Armbruster <armbru@redhat.com> wrote: > >> marcandre.lureau@redhat.com writes: >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> > Instead of building prepocessor conditions from a list of string, use >> > the result generated from QAPISchemaIfCond.cgen(). >> >> I understand why you're doing this, but only because I know where you're >> headed. By itself, it is not an improvement: you move C generation out >> of common.py into schema.py. You need to explain why that's useful. >> >> > What about? > > In the following commits, QAPISchemaIfCond is going to hold an internal > tree structure. Moving cgen() there allows to abstract away the condition > representation. Yes, that's better. [...]
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 6ad1eeb61d..c305aaf2f1 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -12,7 +12,7 @@ # See the COPYING file in the top-level directory. import re -from typing import Match, Optional, Sequence +from typing import Match, Optional #: Magic string that gets removed along with all space to its right. @@ -194,22 +194,20 @@ def guardend(name: str) -> str: name=c_fname(name).upper()) -def gen_if(ifcond: Sequence[str]) -> str: - ret = '' - for ifc in ifcond: - ret += mcgen(''' +def gen_if(cond: str) -> str: + if not cond: + return '' + return mcgen(''' #if %(cond)s -''', cond=ifc) - return ret +''', cond=cond) -def gen_endif(ifcond: Sequence[str]) -> str: - ret = '' - for ifc in reversed(ifcond): - ret += mcgen(''' +def gen_endif(cond: str) -> str: + if not cond: + return '' + return mcgen(''' #endif /* %(cond)s */ -''', cond=ifc) - return ret +''', cond=cond) def must_match(pattern: str, string: str) -> Match[str]: diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 1c5b190276..51a597a025 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str: if added[0] == '\n': out += '\n' added = added[1:] - out += gen_if(ifcond.ifcond) + out += gen_if(ifcond.cgen()) out += added - out += gen_endif(ifcond.ifcond) + out += gen_endif(ifcond.cgen()) return out diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 77a8c33ad4..474b08fd4d 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -124,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.ifcond) + ret += gen_if(obj.ifcond.cgen()) ret += _tree_to_qlit(obj.value, level) if obj.ifcond: - ret += '\n' + gen_endif(obj.ifcond.ifcond) + ret += '\n' + gen_endif(obj.ifcond.cgen()) return ret ret = '' diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index bc357ebbfa..aa4715c519 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -29,6 +29,9 @@ class QAPISchemaIfCond: def __init__(self, ifcond=None): self.ifcond = ifcond or [] + def cgen(self): + return ' && '.join(self.ifcond) + # Returns true if the condition is not void def __bool__(self): return bool(self.ifcond) diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 3673cf0f49..db9ff95bd1 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -51,13 +51,13 @@ def gen_enum_lookup(name: str, ''', c_name=c_name(name)) for memb in members: - ret += gen_if(memb.ifcond.ifcond) + ret += gen_if(memb.ifcond.cgen()) index = c_enum_const(name, memb.name, prefix) ret += mcgen(''' [%(index)s] = "%(name)s", ''', index=index, name=memb.name) - ret += gen_endif(memb.ifcond.ifcond) + ret += gen_endif(memb.ifcond.cgen()) ret += mcgen(''' }, @@ -81,12 +81,12 @@ def gen_enum(name: str, c_name=c_name(name)) for memb in enum_members: - ret += gen_if(memb.ifcond.ifcond) + ret += gen_if(memb.ifcond.cgen()) ret += mcgen(''' %(c_enum)s, ''', c_enum=c_enum_const(name, memb.name, prefix)) - ret += gen_endif(memb.ifcond.ifcond) + ret += gen_endif(memb.ifcond.cgen()) ret += mcgen(''' } %(c_name)s; @@ -126,7 +126,7 @@ def gen_array(name: str, element_type: QAPISchemaType) -> str: def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str: ret = '' for memb in members: - ret += gen_if(memb.ifcond.ifcond) + ret += gen_if(memb.ifcond.cgen()) if memb.optional: ret += mcgen(''' bool has_%(c_name)s; @@ -136,7 +136,7 @@ def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str: %(c_type)s %(c_name)s; ''', c_type=memb.type.c_type(), c_name=c_name(memb.name)) - ret += gen_endif(memb.ifcond.ifcond) + ret += gen_endif(memb.ifcond.cgen()) return ret @@ -159,7 +159,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond, ret += mcgen(''' ''') - ret += gen_if(ifcond.ifcond) + ret += gen_if(ifcond.cgen()) ret += mcgen(''' struct %(c_name)s { ''', @@ -193,7 +193,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond, ret += mcgen(''' }; ''') - ret += gen_endif(ifcond.ifcond) + ret += gen_endif(ifcond.cgen()) return ret @@ -220,13 +220,13 @@ def gen_variants(variants: QAPISchemaVariants) -> str: for var in variants.variants: if var.type.name == 'q_empty': continue - ret += gen_if(var.ifcond.ifcond) + ret += gen_if(var.ifcond.cgen()) ret += mcgen(''' %(c_type)s %(c_name)s; ''', c_type=var.type.c_unboxed_type(), c_name=c_name(var.name)) - ret += gen_endif(var.ifcond.ifcond) + ret += gen_endif(var.ifcond.cgen()) ret += mcgen(''' } u; diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 67721b2470..56ea516399 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -79,7 +79,7 @@ def gen_visit_object_members(name: str, for memb in members: deprecated = 'deprecated' in [f.name for f in memb.features] - ret += gen_if(memb.ifcond.ifcond) + ret += gen_if(memb.ifcond.cgen()) if memb.optional: ret += mcgen(''' if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) { @@ -112,7 +112,7 @@ def gen_visit_object_members(name: str, ret += mcgen(''' } ''') - ret += gen_endif(memb.ifcond.ifcond) + ret += gen_endif(memb.ifcond.cgen()) if variants: tag_member = variants.tag_member @@ -126,7 +126,7 @@ def gen_visit_object_members(name: str, for var in variants.variants: case_str = c_enum_const(tag_member.type.name, var.name, tag_member.type.prefix) - ret += gen_if(var.ifcond.ifcond) + ret += gen_if(var.ifcond.cgen()) if var.type.name == 'q_empty': # valid variant and nothing to do ret += mcgen(''' @@ -142,7 +142,7 @@ def gen_visit_object_members(name: str, case=case_str, c_type=var.type.c_name(), c_name=c_name(var.name)) - ret += gen_endif(var.ifcond.ifcond) + ret += gen_endif(var.ifcond.cgen()) ret += mcgen(''' default: abort(); @@ -228,7 +228,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str: c_name=c_name(name)) for var in variants.variants: - ret += gen_if(var.ifcond.ifcond) + ret += gen_if(var.ifcond.cgen()) ret += mcgen(''' case %(case)s: ''', @@ -254,7 +254,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str: ret += mcgen(''' break; ''') - ret += gen_endif(var.ifcond.ifcond) + ret += gen_endif(var.ifcond.cgen()) ret += mcgen(''' case QTYPE_NONE: