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() and hide the
> implementation details.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@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: