| Message ID | 877fi3s03p.fsf@blackfin.pond.sub.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/17/2016 10:44 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> There's no reason to do two malloc's for a flat union; let's just >> inline the branch struct directly into the C union branch of the >> flat union. >> >> Surprisingly, fewer clients were actually using explicit references >> to the branch types in comparison to the number of flat unions >> thus modified. >> >> This lets us reduce the hack in qapi-types:gen_variants() added in >> the previous patch; we no longer need to distinguish between >> alternates and flat unions. It also lets us get rid of all traces >> of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but >> not all) special cases of simplie unions. > > simple > >> >> Unfortunately, simple unions are not as easy to convert; because >> we are special-casing the hidden implicit type with a single 'data' >> member, we really DO need to keep calling another layer of >> visit_start_struct(), with a second malloc. Hence, >> gen_visit_fields_decl() has to special case implicit types (the >> type for a simple union variant). > > Simple unions should be mere sugar for the equivalent flat union, as > explained in qapi-code-gen.txt. That they aren't now on the C side is > accidental complexity. I hope we can clean that up relatively soon. > > In the long run, I'd like to replace the whole struct / flat union / > simple union mess by a variant record type. We're getting closer :) > >> Note that after this patch, the only remaining use of >> visit_start_implicit_struct() is for alternate types; the next >> couple of patches will do further cleanups based on that fact. > > Remind me, what makes alternates need visit_start_implicit_struct()? Here's what the two functions do: visit_start_struct(): optionally allocate, and consume {} visit_start_implicit_struct(): allocate, but do not consume {} When visiting an alternate, we need to allocate the C struct that contains the various alternate branches (visit_start_implicit_struct(), unchanged by this patch, but renamed visit_start_alternate in 13/13), then within that struct, if the next token is '{' we need to visit the C struct for the object branch of the alternate (pre-series, that was boxed, so we used visit_type_FOO(&obj) which calls visit_start_struct() for a full allocation and consumption of {}; but with the previous patch, it is now already allocated, so we now use visit_type_FOO(NULL) to skip the allocation while still consuming the {}). I can try to work something along the lines of that text into my commit messages for v11. > >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v10: new patch >> >> If anything, we could match our simple union wire format more closely >> by teaching qapi-types to expose implicit types inline, and write: >> >> struct SU { >> SUKind type; >> union { >> struct { >> Branch1 *data; >> } branch1; >> struct { >> Branch2 *data; >> } branch2; >> } u; >> }; >> >> where we would then access su.u.branch1.data->member instead of >> the current su.u.branch1->member. > > Looks like the cleanup I mentioned above. Yay, I'm glad you like it! I've already written the patch for it, but it was big enough (and needs several other prerequisite cleanups in the codebase to use C99 initializers for things like SocketAddress to make the switch easier to review) that I haven't posted it yet. And yes, it completely gets rid of the simple_union_type() hack. >> @@ -144,7 +136,7 @@ def gen_variants(variants): > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > typ = var.simple_union_type() or var.type >> ret += mcgen(''' >> %(c_type)s %(c_name)s; >> ''', >> - c_type=typ.c_type(is_member=inline), >> + c_type=typ.c_type(is_member=not var.simple_union_type()), >> c_name=c_name(var.name)) > > This is where we generate flat union members unboxed: is_member=True > suppresses the pointer suffix. Still dislike the name is_member :) > > Perhaps: > > # Ugly special case for simple union TODO get rid of it > simple_union_type = var.simple_union_type() > typ = simple_union_type or var.type > ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > c_type=typ.c_type(is_member=not simple_union_type), > c_name=c_name(var.name)) > > Slightly more readable, and makes it more clear that "ugly special case" > applies to is_member=... as well. It gets renamed to is_unboxed after the review on 10/13. But even after my patch to convert simple unions, this code will still be c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework .c_type() to not need two separate boolean flags for the three different contexts in which we use a type name (declaring an unboxed member to a struct, declaring a local variable, and declaring a const parameter). >> static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp); >> ''', >> - c_type=typ.c_name()) >> - struct_fields_seen.add(typ.name) >> - return ret > > Two changes squashed together. First step is mere style: Then I'll split into two patches for v11. > Second step is the actual change: > > @@ -35,7 +39,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error ** > > > def gen_visit_fields_decl(typ): > - if typ.name in struct_fields_seen: > + if typ.is_implicit() or typ.name in struct_fields_seen: > return '' > struct_fields_seen.add(typ.name) > > Much easier to see what's going on now. > > I guess you add .is_implicit() here so that gen_visit_object() can call > it unconditionally. It's odd; other gen_ functions don't check > .is_implicit(). Although I have more plans to use .is_implicit() - I have patches in my local tree that allow: { 'enum': 'Enum', 'data': [ 'branch' ] } { 'union': 'U', 'base': { 'anonymous1': 'Enum' }, 'discriminator': 'anonymous1', 'data': { 'branch': { 'anonymous2': 'str' } } } that is, both an anonymous base, and an anonymous branch. It results in more places where we'll need to suppress declarations if the type is implicit; so doing it here instead of in each caller proves easier in the long run. But for this patch, I can probably go along with your idea of keeping the complexity in the caller, where we highlight that simple unions are still special cases for a bit longer... >> @@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants): >> >> if variants: >> for var in variants.variants: >> - # Ugly special case for simple union TODO get rid of it >> - if not var.simple_union_type(): >> - ret += gen_visit_implicit_struct(var.type) >> + ret += gen_visit_fields_decl(var.type) > > Before: if this is a flat union member of type FOO, we're going to call > visit_type_implicit_FOO(), as you can see in the next hunk. Ensure it's > in scope by generating it unless it's been generated already. > > After: we're going to call visit_type_FOO_fields() instead. Generate a > forward declaration unless either the function or the forward > declaration has been generated already. Except don't generate it when > FOO is an implicit type, because then the member is simple rather than > flat. > > Doesn't this unduly hide the ugly special case? > > To keep it in view, I'd write > > # Ugly special case for simple union TODO get rid of it > if not var.simple_union_type(): > - ret += gen_visit_implicit_struct(var.type) > + ret += gen_visit_fields_decl(var.type) > > and drop the .is_implicit() from gen_visit_fields_decl(). > > Would this work? ...It should; I'm testing it now. > > Every time I come across "implicit" structs, I get confused, and have to > dig to unconfuse myself. Good to get rid of one. Yep - and it makes my stalled work on documenting visitor.h easier with fewer ugly things to document :) >> case CPU_INFO_ARCH_TRICORE: >> - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore->PC); >> + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC); >> break; >> default: >> break; > > That's not bad at all. I was actually pleasantly shocked at how few places in code needed changing. The conversion of simple unions sitting in my local tree was more complex (much of that because we use SocketAddress in a LOT more places).
Eric Blake <eblake@redhat.com> writes: > On 02/17/2016 10:44 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> There's no reason to do two malloc's for a flat union; let's just >>> inline the branch struct directly into the C union branch of the >>> flat union. >>> >>> Surprisingly, fewer clients were actually using explicit references >>> to the branch types in comparison to the number of flat unions >>> thus modified. >>> >>> This lets us reduce the hack in qapi-types:gen_variants() added in >>> the previous patch; we no longer need to distinguish between >>> alternates and flat unions. It also lets us get rid of all traces >>> of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but >>> not all) special cases of simplie unions. >> >> simple >> >>> >>> Unfortunately, simple unions are not as easy to convert; because >>> we are special-casing the hidden implicit type with a single 'data' >>> member, we really DO need to keep calling another layer of >>> visit_start_struct(), with a second malloc. Hence, >>> gen_visit_fields_decl() has to special case implicit types (the >>> type for a simple union variant). >> >> Simple unions should be mere sugar for the equivalent flat union, as >> explained in qapi-code-gen.txt. That they aren't now on the C side is >> accidental complexity. I hope we can clean that up relatively soon. >> >> In the long run, I'd like to replace the whole struct / flat union / >> simple union mess by a variant record type. > > We're getting closer :) > >> >>> Note that after this patch, the only remaining use of >>> visit_start_implicit_struct() is for alternate types; the next >>> couple of patches will do further cleanups based on that fact. >> >> Remind me, what makes alternates need visit_start_implicit_struct()? > > Here's what the two functions do: > > visit_start_struct(): optionally allocate, and consume {} > visit_start_implicit_struct(): allocate, but do not consume {} > > When visiting an alternate, we need to allocate the C struct that > contains the various alternate branches (visit_start_implicit_struct(), > unchanged by this patch, but renamed visit_start_alternate in 13/13), > then within that struct, if the next token is '{' we need to visit the C > struct for the object branch of the alternate (pre-series, that was > boxed, so we used visit_type_FOO(&obj) which calls visit_start_struct() > for a full allocation and consumption of {}; but with the previous > patch, it is now already allocated, so we now use visit_type_FOO(NULL) > to skip the allocation while still consuming the {}). > > I can try to work something along the lines of that text into my commit > messages for v11. I've since made it to PATCH 13, and found the fused visit_start_alternate(). Much easier to comprehend than the visit_start_implicit_struct() + visit_get_next_type() combo. >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> --- >>> v10: new patch >>> >>> If anything, we could match our simple union wire format more closely >>> by teaching qapi-types to expose implicit types inline, and write: >>> >>> struct SU { >>> SUKind type; >>> union { >>> struct { >>> Branch1 *data; >>> } branch1; >>> struct { >>> Branch2 *data; >>> } branch2; >>> } u; >>> }; >>> >>> where we would then access su.u.branch1.data->member instead of >>> the current su.u.branch1->member. >> >> Looks like the cleanup I mentioned above. > > Yay, I'm glad you like it! I've already written the patch for it, but it > was big enough (and needs several other prerequisite cleanups in the > codebase to use C99 initializers for things like SocketAddress to make > the switch easier to review) that I haven't posted it yet. And yes, it > completely gets rid of the simple_union_type() hack. Looking forward to its demise. >>> @@ -144,7 +136,7 @@ def gen_variants(variants): >> for var in variants.variants: >> # Ugly special case for simple union TODO get rid of it >> typ = var.simple_union_type() or var.type >>> ret += mcgen(''' >>> %(c_type)s %(c_name)s; >>> ''', >>> - c_type=typ.c_type(is_member=inline), >>> + c_type=typ.c_type(is_member=not var.simple_union_type()), >>> c_name=c_name(var.name)) >> >> This is where we generate flat union members unboxed: is_member=True >> suppresses the pointer suffix. Still dislike the name is_member :) >> >> Perhaps: >> >> # Ugly special case for simple union TODO get rid of it >> simple_union_type = var.simple_union_type() >> typ = simple_union_type or var.type >> ret += mcgen(''' >> %(c_type)s %(c_name)s; >> ''', >> c_type=typ.c_type(is_member=not simple_union_type), >> c_name=c_name(var.name)) >> >> Slightly more readable, and makes it more clear that "ugly special case" >> applies to is_member=... as well. > > It gets renamed to is_unboxed after the review on 10/13. But even after > my patch to convert simple unions, this code will still be > c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework > .c_type() to not need two separate boolean flags for the three different > contexts in which we use a type name (declaring an unboxed member to a > struct, declaring a local variable, and declaring a const parameter). A possible alternative to a single c_type() with flags for context would be separate c_CONTEXT_type(). In QAPISchemaType: def c_type(self): pass def c_param_type(self): return self.c_type() QAPISchemaBuiltinType overrides: 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 QAPISchemaEnumType: def c_type(self): return c_name(self.name) QAPISchemaArrayType: def c_type(self): return c_name(self.name) + pointer_suffix QAPISchemaObjectType: def c_type(self): assert not self.is_implicit() return c_name(self.name) + pointer_suffix def c_unboxed_type(self): return c_name(self.name) QAPISchemaAlternateType: def c_type(self): return c_name(self.name) + pointer_suffix Lots of trivial code, as so often with OO. >>> static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp); >>> ''', >>> - c_type=typ.c_name()) >>> - struct_fields_seen.add(typ.name) >>> - return ret >> >> Two changes squashed together. First step is mere style: > > Then I'll split into two patches for v11. > >> Second step is the actual change: >> >> @@ -35,7 +39,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error ** >> >> >> def gen_visit_fields_decl(typ): >> - if typ.name in struct_fields_seen: >> + if typ.is_implicit() or typ.name in struct_fields_seen: >> return '' >> struct_fields_seen.add(typ.name) >> >> Much easier to see what's going on now. >> >> I guess you add .is_implicit() here so that gen_visit_object() can call >> it unconditionally. It's odd; other gen_ functions don't check >> .is_implicit(). > > Although I have more plans to use .is_implicit() - I have patches in my > local tree that allow: > > { 'enum': 'Enum', 'data': [ 'branch' ] } > { 'union': 'U', 'base': { 'anonymous1': 'Enum' }, > 'discriminator': 'anonymous1', > 'data': { 'branch': { 'anonymous2': 'str' } } } > > that is, both an anonymous base, and an anonymous branch. It results in > more places where we'll need to suppress declarations if the type is > implicit; so doing it here instead of in each caller proves easier in > the long run. > > But for this patch, I can probably go along with your idea of keeping > the complexity in the caller, where we highlight that simple unions are > still special cases for a bit longer... Yes, please. >>> @@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants): >>> >>> if variants: >>> for var in variants.variants: >>> - # Ugly special case for simple union TODO get rid of it >>> - if not var.simple_union_type(): >>> - ret += gen_visit_implicit_struct(var.type) >>> + ret += gen_visit_fields_decl(var.type) >> >> Before: if this is a flat union member of type FOO, we're going to call >> visit_type_implicit_FOO(), as you can see in the next hunk. Ensure it's >> in scope by generating it unless it's been generated already. >> >> After: we're going to call visit_type_FOO_fields() instead. Generate a >> forward declaration unless either the function or the forward >> declaration has been generated already. Except don't generate it when >> FOO is an implicit type, because then the member is simple rather than >> flat. >> >> Doesn't this unduly hide the ugly special case? >> >> To keep it in view, I'd write >> >> # Ugly special case for simple union TODO get rid of it >> if not var.simple_union_type(): >> - ret += gen_visit_implicit_struct(var.type) >> + ret += gen_visit_fields_decl(var.type) >> >> and drop the .is_implicit() from gen_visit_fields_decl(). >> >> Would this work? > > ...It should; I'm testing it now. > >> >> Every time I come across "implicit" structs, I get confused, and have to >> dig to unconfuse myself. Good to get rid of one. > > Yep - and it makes my stalled work on documenting visitor.h easier with > fewer ugly things to document :) I welcome a smaller visitor.h; reviewing the first iteration of the documentation patch was tough going. >>> case CPU_INFO_ARCH_TRICORE: >>> - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore->PC); >>> + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC); >>> break; >>> default: >>> break; >> >> That's not bad at all. > > I was actually pleasantly shocked at how few places in code needed > changing. The conversion of simple unions sitting in my local tree was > more complex (much of that because we use SocketAddress in a LOT more > places).
On 02/18/2016 01:51 AM, Markus Armbruster wrote: >> It gets renamed to is_unboxed after the review on 10/13. But even after >> my patch to convert simple unions, this code will still be >> c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework >> .c_type() to not need two separate boolean flags for the three different >> contexts in which we use a type name (declaring an unboxed member to a >> struct, declaring a local variable, and declaring a const parameter). > > A possible alternative to a single c_type() with flags for context would > be separate c_CONTEXT_type(). > > In QAPISchemaType: > > def c_type(self): > pass > > def c_param_type(self): > return self.c_type() and def c_unboxed_type(self): return self.c_type() so that c_unboxed_type() is callable on all types, not just objects. > > QAPISchemaBuiltinType overrides: > > 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 ... > > Lots of trivial code, as so often with OO. But I'm liking it a bit better than the two flags. Your suggestion came after my v11; at this point, if you want me to pursue the idea, I'm glad to do it as a followup, and include it in my next series where I finish the conversion of simple unions.
Eric Blake <eblake@redhat.com> writes: > On 02/18/2016 01:51 AM, Markus Armbruster wrote: > >>> It gets renamed to is_unboxed after the review on 10/13. But even after >>> my patch to convert simple unions, this code will still be >>> c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework >>> .c_type() to not need two separate boolean flags for the three different >>> contexts in which we use a type name (declaring an unboxed member to a >>> struct, declaring a local variable, and declaring a const parameter). >> >> A possible alternative to a single c_type() with flags for context would >> be separate c_CONTEXT_type(). >> >> In QAPISchemaType: >> >> def c_type(self): >> pass >> >> def c_param_type(self): >> return self.c_type() > > and > > def c_unboxed_type(self): > return self.c_type() > > so that c_unboxed_type() is callable on all types, not just objects. I defined it only for object types because we box only object types. Err, object types and lists. If we have a need to call it for arbitrary types, we can do that, we just need to pick names carefully, and write suitable contracts. >> >> QAPISchemaBuiltinType overrides: >> >> 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 > ... > >> >> Lots of trivial code, as so often with OO. > > But I'm liking it a bit better than the two flags. Your suggestion came > after my v11; at this point, if you want me to pursue the idea, I'm glad > to do it as a followup, and include it in my next series where I finish > the conversion of simple unions. I need to run an errand now. When I'm back, I'll review whether the series can go to qapi-next without a respin.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 948bde4..ac7d504 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -39,15 +39,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error ** def gen_visit_fields_decl(typ): - ret = '' - if typ.name not in struct_fields_seen: - ret += mcgen(''' + if typ.name in struct_fields_seen: + return '' + struct_fields_seen.add(typ.name) + + return mcgen(''' static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp); ''', c_type=typ.c_name()) - struct_fields_seen.add(typ.name) - return ret def gen_visit_implicit_struct(typ):