| Message ID | 1456443528-13901-20-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > QAPISchemaType.c_type() was a bit awkward. Rather than having two > optional orthogonal boolean flags that should never both be true, > and where all callers pass a compile-time constant, provide three > different method names that can be overridden as needed, and where > the caller just uses the right variant. It requires slightly more > Python, but is arguably easier to read. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: no change > --- > scripts/qapi.py | 38 +++++++++++++++++++++++++++++--------- > scripts/qapi-types.py | 2 +- > 2 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index d5f4e28..32f37d8 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -826,8 +826,17 @@ class QAPISchemaVisitor(object): > > > class QAPISchemaType(QAPISchemaEntity): > - def c_type(self, is_param=False, is_unboxed=False): > - return c_name(self.name) + pointer_suffix > + # Normal usage of the type, such as declaring a local variable > + def c_type(self): > + pass > + > + # Use of a type in a parameter list > + def c_param_type(self): > + return self.c_type() These two make sense for any type. The fact that we override c_param_type() only for object types is detail. > + > + # Use of a type in a struct declaration > + def c_unboxed_type(self): > + return self.c_type() This one I find bit problematic. We only ever box object types, but those we box almost everywhere. This method gets used in the select places where we don't box them (currently just one). I'm afraid "used in a struct declaration" isn't a good definition. I initially suggested to define c_unboxed_type() only for QAPISchemaObjectType because there boxed vs. unboxed makes sense. However, your code would like to call it for arbitrary types. Perhaps we can cure my belly-ache with nothing more than carefully drafted function contracts. Let me try. # Return the C type for common use. # For the types we commonly box, this is a pointer type. def c_type(self): # Return the C type to be used in a parameter list. def c_param_type(self): # Return the C type to be used where we suppress boxing. def c_unboxed_type(self): > > def c_null(self): > return 'NULL' > @@ -859,8 +868,11 @@ class QAPISchemaBuiltinType(QAPISchemaType): > def c_name(self): > return self.name > > - def c_type(self, is_param=False, is_unboxed=False): > - if is_param and self.name == 'str': > + def c_type(self): > + return self._c_type_name > + > + def c_param_type(self): > + if self.name == 'str': > return 'const ' + self._c_type_name > return self._c_type_name > > @@ -893,7 +905,7 @@ class QAPISchemaEnumType(QAPISchemaType): > # See QAPISchema._make_implicit_enum_type() > return self.name.endswith('Kind') > > - def c_type(self, is_param=False, is_unboxed=False): > + def c_type(self): > return c_name(self.name) > > def member_names(self): > @@ -925,6 +937,9 @@ class QAPISchemaArrayType(QAPISchemaType): > def is_implicit(self): > return True > > + def c_type(self): > + return c_name(self.name) + pointer_suffix > + > def json_type(self): > return 'array' > > @@ -994,12 +1009,14 @@ class QAPISchemaObjectType(QAPISchemaType): > assert not self.is_implicit() > return QAPISchemaType.c_name(self) > > - def c_type(self, is_param=False, is_unboxed=False): > + def c_type(self): > assert not self.is_implicit() > - if is_unboxed: > - return c_name(self.name) > return c_name(self.name) + pointer_suffix > > + def c_unboxed_type(self): > + assert not self.is_implicit() > + return c_name(self.name) > + > def json_type(self): > return 'object' > > @@ -1140,6 +1157,9 @@ class QAPISchemaAlternateType(QAPISchemaType): > for v in self.variants.variants: > v.check_clash(self.info, seen) > > + def c_type(self): > + return c_name(self.name) + pointer_suffix > + > def json_type(self): > return 'value' > > @@ -1634,7 +1654,7 @@ def gen_params(arg_type, extra): > sep = ', ' > if memb.optional: > ret += 'bool has_%s, ' % c_name(memb.name) > - ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name)) > + ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name)) > if extra: > ret += sep + extra > return ret > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 161443e..ce6d6f9 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -140,7 +140,7 @@ def gen_variants(variants): > ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > - c_type=var.type.c_type(is_unboxed=True), > + c_type=var.type.c_unboxed_type(), > c_name=c_name(var.name)) > > ret += mcgen(''' Looks good otherwise.
On 03/03/2016 06:29 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> QAPISchemaType.c_type() was a bit awkward. Rather than having two >> optional orthogonal boolean flags that should never both be true, >> and where all callers pass a compile-time constant, provide three >> different method names that can be overridden as needed, and where >> the caller just uses the right variant. It requires slightly more >> Python, but is arguably easier to read. >> >> + >> + # Use of a type in a struct declaration >> + def c_unboxed_type(self): >> + return self.c_type() > > This one I find bit problematic. We only ever box object types, but > those we box almost everywhere. This method gets used in the select > places where we don't box them (currently just one). I'm afraid "used > in a struct declaration" isn't a good definition. > > I initially suggested to define c_unboxed_type() only for > QAPISchemaObjectType because there boxed vs. unboxed makes sense. > However, your code would like to call it for arbitrary types. qapi-types.py is calling it for arbitrary types thanks to QAPI alternates. If I didn't put it here, then the code there needs to special-case for isinstance(type, QAPISchemaObjectType) before calling .c_unboxed_type(), and fall back to .c_type() otherwise. > > Perhaps we can cure my belly-ache with nothing more than carefully > drafted function contracts. Let me try. > > # Return the C type for common use. > # For the types we commonly box, this is a pointer type. > def c_type(self): > > # Return the C type to be used in a parameter list. > def c_param_type(self): > > # Return the C type to be used where we suppress boxing. > def c_unboxed_type(self): Sure, that helps.
diff --git a/scripts/qapi.py b/scripts/qapi.py index d5f4e28..32f37d8 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -826,8 +826,17 @@ class QAPISchemaVisitor(object): class QAPISchemaType(QAPISchemaEntity): - def c_type(self, is_param=False, is_unboxed=False): - return c_name(self.name) + pointer_suffix + # Normal usage of the type, such as declaring a local variable + def c_type(self): + pass + + # Use of a type in a parameter list + def c_param_type(self): + return self.c_type() + + # Use of a type in a struct declaration + def c_unboxed_type(self): + return self.c_type() def c_null(self): return 'NULL' @@ -859,8 +868,11 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_name(self): return self.name - def c_type(self, is_param=False, is_unboxed=False): - if is_param and self.name == 'str': + def c_type(self): + return self._c_type_name + + def c_param_type(self): + if self.name == 'str': return 'const ' + self._c_type_name return self._c_type_name @@ -893,7 +905,7 @@ class QAPISchemaEnumType(QAPISchemaType): # See QAPISchema._make_implicit_enum_type() return self.name.endswith('Kind') - def c_type(self, is_param=False, is_unboxed=False): + def c_type(self): return c_name(self.name) def member_names(self): @@ -925,6 +937,9 @@ class QAPISchemaArrayType(QAPISchemaType): def is_implicit(self): return True + def c_type(self): + return c_name(self.name) + pointer_suffix + def json_type(self): return 'array' @@ -994,12 +1009,14 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.is_implicit() return QAPISchemaType.c_name(self) - def c_type(self, is_param=False, is_unboxed=False): + def c_type(self): assert not self.is_implicit() - if is_unboxed: - return c_name(self.name) return c_name(self.name) + pointer_suffix + def c_unboxed_type(self): + assert not self.is_implicit() + return c_name(self.name) + def json_type(self): return 'object' @@ -1140,6 +1157,9 @@ class QAPISchemaAlternateType(QAPISchemaType): for v in self.variants.variants: v.check_clash(self.info, seen) + def c_type(self): + return c_name(self.name) + pointer_suffix + def json_type(self): return 'value' @@ -1634,7 +1654,7 @@ def gen_params(arg_type, extra): sep = ', ' if memb.optional: ret += 'bool has_%s, ' % c_name(memb.name) - ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name)) + ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name)) if extra: ret += sep + extra return ret diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 161443e..ce6d6f9 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -140,7 +140,7 @@ def gen_variants(variants): ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=var.type.c_type(is_unboxed=True), + c_type=var.type.c_unboxed_type(), c_name=c_name(var.name)) ret += mcgen('''
QAPISchemaType.c_type() was a bit awkward. Rather than having two optional orthogonal boolean flags that should never both be true, and where all callers pass a compile-time constant, provide three different method names that can be overridden as needed, and where the caller just uses the right variant. It requires slightly more Python, but is arguably easier to read. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: no change --- scripts/qapi.py | 38 +++++++++++++++++++++++++++++--------- scripts/qapi-types.py | 2 +- 2 files changed, 30 insertions(+), 10 deletions(-)