Eric Blake <[email protected]> writes: > On 03/08/2016 09:23 AM, Markus Armbruster wrote: >> Eric Blake <[email protected]> writes: >> >>> Rather than requiring all flat unions to explicitly create >>> a separate base struct, we can allow the qapi schema to specify >>> the common members via an inline dictionary. This is similar to >>> how commands can specify an inline anonymous type for its 'data'. >>> We already have several struct types that only exist to serve as >>> a single flat union's base; the next commit will clean them up. >>> > >> >> I think you also >> >> * fixed a missing allow_optional=True in check_union() > > More on that below. > >> >> * fixed a missing "non-optional" in qapi-code-gen.txt (mention in commit >> message? separate patch?) >> >> * renamed readonly to read-only in an example in qapi-code-gen.txt to be >> closer to the code (separate patch?) > > Could separate those two cleanups if it helps.
I'd like the fix recorded in git-log. For that, a mention in the commit message would suffice, but I think I'd rather separate them. >>> @@ -560,9 +562,10 @@ def check_union(expr, expr_info): >>> >>> # Else, it's a flat union. >>> else: >>> - # The object must have a string member 'base'. >>> + # The object must have a string or dictionary 'base'. >>> check_type(expr_info, "'base' for union '%s'" % name, >>> - base, allow_metas=['struct']) >>> + base, allow_dict=True, allow_optional=True, >>> + allow_metas=['struct']) >> >> This is where you added allow_optional=True. > > allow_optional only matters if allow_dict is True. We have places where > we allow a dict but do not allow optionals (namely, simple unions); but > where we are creating an anonymous type, we want to allow optionals at > the same time we allow a dict. I think this is just a case where the > commit message needs to be beefed up. Perhaps not even that. I'm spoiled by your meticulous patch versioning, and when once in a blue moon I spot something you didn't announce there, I take note. >>> +A flat union definition avoids nesting on the wire, and specifies a >>> +set of common members that occur in all variants of the union. The >>> +'base' key must specifiy either a type name (the type must be a >>> +struct, not a union), or a dictionary representing an anonymous type. >>> +All branches of the union must be complex types, and the top-level >>> +members of the union dictionary on the wire will be combination of >>> +members from both the base type and the appropriate branch type (when >>> +merging two dictionaries, there must be no keys in common). The >>> +'discriminator' member must be the name of a non-optional enum-typed >> >> This is where you supplied the missing "non-optional". >> >>> +member of the base struct. >>> >> >> And below, you rename readonly to read-only. > > They touch the same paragraph, but I can separate them if it would make > this patch feel cleaner.
