Hi
On Mon, Aug 2, 2021 at 6:46 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Instead of building prepocessor conditions from a list of string, use
> > the result generated from QAPISchemaIfCond.cgen() and hide the
> > implementation details.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> Please mention that the patch changes generated code. See below for
> details.
>
I'll add
Note: this patch introduces a minor regression, generating a redundant
pair of parenthesis. This is fixed in a later patch in this
series ("qapi: replace if condition list with dict [..]")
> > ---
> > scripts/qapi/common.py | 35 ++++++++++++++++++++++-------------
> > scripts/qapi/gen.py | 4 ++--
> > scripts/qapi/introspect.py | 4 ++--
> > scripts/qapi/schema.py | 5 ++++-
> > scripts/qapi/types.py | 20 ++++++++++----------
> > scripts/qapi/visit.py | 12 ++++++------
> > 6 files changed, 46 insertions(+), 34 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 6ad1eeb61d..ba9fe14e4b 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -12,7 +12,12 @@
> > # See the COPYING file in the top-level directory.
> >
> > import re
> > -from typing import Match, Optional, Sequence
> > +from typing import (
> > + List,
> > + Match,
> > + Optional,
> > + Union,
> > +)
> >
> >
> > #: Magic string that gets removed along with all space to its right.
> > @@ -194,22 +199,26 @@ 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 cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > + if not ifcond:
> > + return ''
> > + return '(' + ') && ('.join(ifcond) + ')'
> > +
> > +
> > +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)
> >
> >
>
> This patch does three things:
>
> (1) Change gen_if(), gen_endif() to always generate a single #if,
> #endif. This enables:
>
> (2) Factor cgen_ifcond() out of gen_if() and gen_endif()
>
> (3) Lift the call of cgen_ifcond() into into gen_if()'s, gen_endif()'s
> callers.
>
> I'd split the patch. This is *not* a demand.
>
> The motivation for (3) is unclear. Is it so gen_if() doesn't depend on
> QAPISchemaIfCond?
>
> Step (1) affects the generated code. When @ifcond is [COND1, COND2, ...],
> gen_if()'s value changes from
>
> #if COND1
> #if COND2
> ...
>
> to
>
> #if (COND1) && (COND2)
>
> Example: in tests/test-qapi-introspect.c
>
> @@ -259,11 +257,9 @@ const QLitObject test_qmp_schema_qlit =
> QLIT_QDICT(((QLitDictEntry[]) {
> { "arg-type", QLIT_QSTR("1"), },
> { "features", QLIT_QLIST(((QLitObject[]) {
> -#if defined(TEST_IF_COND_1)
> -#if defined(TEST_IF_COND_2)
> +#if (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2))
> QLIT_QSTR("feature1"),
> -#endif /* defined(TEST_IF_COND_2) */
> -#endif /* defined(TEST_IF_COND_1) */
> +#endif /* (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2)) */
> {}
> })), },
> { "meta-type", QLIT_QSTR("command"), },
>
> The common case: when it's just [COND], the value changes from
>
> #if COND
>
> to
>
> #if (COND)
>
> which is a bit ugly.
>
> Example: in qapi-types-block-export.c
>
> @@ -76,7 +76,7 @@ const QEnumLookup FuseExportAllowOther_l
> .size = FUSE_EXPORT_ALLOW_OTHER__MAX
> };
>
> -#if defined(CONFIG_FUSE)
> +#if (defined(CONFIG_FUSE))
> void qapi_free_BlockExportOptionsFuse(BlockExportOptionsFuse *obj)
> {
> Visitor *v;
> @@ -89,7 +89,7 @@ void qapi_free_BlockExportOptionsFuse(Bl
> visit_type_BlockExportOptionsFuse(v, NULL, &obj, NULL);
> visit_free(v);
> }
> -#endif /* defined(CONFIG_FUSE) */
> +#endif /* (defined(CONFIG_FUSE)) */
>
> void qapi_free_NbdServerAddOptions(NbdServerAddOptions *obj)
> {
>
> Avoiding the redundant pair of parenthesis takes another special case.
> Let's do it.
>
> > 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 c35fa3bf51..70120f0dcc 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -19,7 +19,7 @@
> > import re
> > from typing import Optional
> >
> > -from .common import POINTER_SUFFIX, c_name
> > +from .common import POINTER_SUFFIX, c_name, cgen_ifcond
> > from .error import QAPIError, QAPISemError, QAPISourceError
> > from .expr import check_exprs
> > from .parser import QAPISchemaParser
> > @@ -29,6 +29,9 @@ class QAPISchemaIfCond:
> > def __init__(self, ifcond=None):
> > self.ifcond = ifcond or []
> >
> > + def cgen(self):
> > + return cgen_ifcond(self.ifcond)
> > +
>
> As far as I can tell, this is only ever used like
>
> gen_if(obj.ifcond.cgen())
>
> and
>
> gen_endif(obj.ifcond.cgen())
>
> Wouldn't
>
> obj.ifcond.gen_if()
>
> and
>
> obj.ifcond.gen_endif()
>
> be neater?
>
Yes
> Not a demand, since we can get there on top if we want to.
>
> > def is_present(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:
>
>
