Eric Blake <ebl...@redhat.com> writes:
> On 07/30/2015 01:11 AM, Markus Armbruster wrote:
>
>>> Another name collision bug: our code generates flat unions as:
>>>
>>> struct BlockdevOptions {
>>> BlockdevDriver driver;
>>> ...
>>> /* End fields inherited from BlockdevOptionsBase. */
>>> /* union tag is BlockdevDriver driver */
>>> union {
>>> void *data;
>>> BlockdevOptionsArchipelago *archipelago;
>>> ...
>>>
>>> which means that if we name any of the branches 'data' (that is, if
>>> 'data' is a member of the enum discriminator), things fail to compile.
>>> We could probably fix that by naming our dummy branch '_data'.
>>
>> I wonder whether member data is actually used. I'll find out.
>
> The dealloc visitor uses 'data' being non-null as a flag on whether to
> deallocate the union even if the tag was invalid for some reason; or
> more importantly, if parsing consumed the tag but then detected an error
> while parsing the union, leaving the union branch partially allocated.
> To avoid a leak, we have to deallocate the branch.
>
> But if the tag was invalid, then why did we ever allocate the union in
> the first place, and how do we prove we are calling the correct free-ing
> function? And if the tag is valid, why can't we just guarantee that the
> union is 0-initialized and that deleting the branch will work through
> the correct branch type instead of worrying about 'data'?
Good questions. Someone will have to review and fix this code. Let's
add a FIXME so we don't forget. Care to propose one?
> We still need a dummy member if it is valid to do { 'union':'Foo',
> 'data':{} } since C doesn't like empty unions, but an empty union seems
> like something we may want to reject, at which point you are probably
> right that deleting the data member altogether should work and still let
> us recover from bad partial parses without a leak.
Either we reject empty unions, or we detect them and add a dummy,
similar to what we do for structs since commit 83ecb22. I figure simply
rejecting them is easier.
