Eric Blake <ebl...@redhat.com> writes:
> On 02/17/2016 10:44 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@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 <ebl...@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).
