On 09/29/2015 06:33 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Expose some weaknesses in the generator: we don't always forbid >> the generation of structs that contain multiple members that map >> to the same C name. > > C or QMP name, perhaps?
Lots of good advice for an improved commit message and test comments;
looks like I'll do a v7 respin to address them.
>
>> Oddly enough, while commit 1e6c1616 fixed a type 2b collision
>> with 'base',
>
> What collided with 'base' before 1e6c1616 and not after?
Uggh. I wrote that comment late at night, and it shows. I was mixing up
struct bases (which are boxed behind a 'FOO *base' member) and flat
unions; but flat union base classes have always been expressed on a
per-member basis. I'll either drop this paragraph entirely, or come up
with a real example (about the only one that might be left is when we
dropped the mis-use of 'kind', we were then introducing the possibility
of collision with the 'discriminator':'name' - and that's one of the two
tests added in this commit where we assert).
>
>> it introduced a different collision that was not
>> previously possible: now that the base members are no longer
>> boxed behind 'base', they can collide with the enum tag members
>> (used as member names within the embedded C union).
>>
>> Ultimately, if we need to have a flat union where an enum
>> value clashes with a base member name, we could change the
>
> Suggest "an enum tag member", because this is confusing enough without
> using multiple names for the same thing :)
>
>> generator to use something like '_tag_value' instead of
>> 'value' for the C name of union members;
>
> Or wrap the base in a struct, or give the union member a name.
Giving the union member a name might be the least amount of typing, but
would still touch quite a bit of code if we have to do it.
>
>> but until such a
>> need arises, it will probably be easier to just forbid these
>> collisions.
>
> Again, I'm not sure this makes sense to readers who aren't already
> familiar with our name clashing issues.
>
> Do you fix any of this later in your series? If yes, you could punt
> detailed bug descriptions to the patches that fixes them.
Yes, I fix a number of the issues later on (although only the two
assertion failures get fixed in subset A; the rest of the fixups are in
the tail end of v5 which will become subset B).
>
>> Some of these collisions are already caught in the old-style
>> parser checks, but ultimately we want all collisions to be
>> caught in the new-style QAPISchema*.check() methods.
>>
>> Some of these negative tests will be deleted later, and positive
>> tests added to qapi-schema-test.json in their place, when the
>> generator code is reworked so that a particular collision no
>> longer causes failed code generation.
>>
>> [git rename detection may be a bit confused by the actions of
>> this commit, since it both creates and renames files that are
>> equally small and similar in content]
>
> Why square brackets?
>
To delineate that this paragraph is is unrelated to the contents of the
patch, but instead guides a reader to how to interpret the git diff of
the patch - if rename detection is enabled, git botches the rename
detection, doing things like:
>> ---
>> tests/Makefile | 10 +++++++++-
>> .../{flat-union-branch-clash.out => args-name-clash.err} | 0
claiming this file is a rename (in reality, I renamed
flat-union-branch-clash.out to flat-union-clash-branch.out; and .out
files never really rename to .err files other than the fact that both
happened to be empty).
We could drop it if desired, but I think it makes sense to keep the
comment in qemu.git (because 'git diff' will botch the rename detection
no matter how much in the future you are inspecting this patch).
>> +++ b/tests/qapi-schema/args-name-clash.json
>> @@ -0,0 +1,2 @@
>> +# FIXME - we should reject data with members that clash when mapped to C
>> names
>> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
>
> Here, the collision is fairly obvious. We could spell it out anyway:
>
> # Multiple members mapping to the same C identifier: both 'a-b' and
> # 'a_b' map to a_b'
> # FIXME they clash in generated C, should reject them cleanly instead.
>
> Unfortunately, the test doesn't actually make the clash visible. Same
> for all the other tests that produce clashes in C or QMP without
> actually upsetting the generator. Oh well, good enough.
And for every FIXME I add here, a later patch in the v5 series resolves
it (either by making it a parse error, or by removing the generated
collision).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
