On 2018-05-10 15:18, Eric Blake wrote: > On 05/10/2018 08:12 AM, Eric Blake wrote: > > Oh, I just had a thought: > >>> +++ b/scripts/qapi/visit.py >>> @@ -40,10 +40,20 @@ def gen_visit_object_members(name, base, members, > >>> if variants: >>> + if variants.default_tag_value is None: >>> + ret += mcgen(''' >>> + %(c_name)s = obj->%(c_name)s; >>> +''', >>> + c_name=c_name(variants.tag_member.name)) >>> + else: >>> + ret += mcgen(''' >>> + if (obj->has_%(c_name)s) { >>> + %(c_name)s = obj->%(c_name)s; >>> + } else { >>> + %(c_name)s = %(enum_const)s; > > In this branch of code, is it worth also generating: > > %has_(c_name)s = true;
You mean obj->has_%(c_name)s, and then also set obj->%(c_name)s? > That way, the rest of the C code does not have to check > has_discriminator, because the process of assigning the default will > ensure that has_discriminator is always true later on. It does have the > effect that output would never omit the discriminator - but that might > be a good thing: if we ever have an output union that used to have a > mandatory discriminator and want to now make it optional, we don't want > to break older clients that expected the discriminator to be present. It > does obscure whether input relied on the default, but I don't think that > hurts. Also, it would make code a bit simpler because it can cover the !has_X case under X == default_X. But OTOH, you could make that case for any optional value -- except where "is missing" has special value. And that's the tricky part: I think it's hard to say that a missing discriminator never has special meaning. We only need the default-variant so we know which variant of the union to pick; but we don't know that the fact that the discriminator value was missing does not have special meaning. Of course, we can simply foreclose that by setting it here. I don't have money in this game, so I suppose it's yours and Markus's call. :-) Max
signature.asc
Description: OpenPGP digital signature