Re: [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4967 bytes --]

On 09/29/2015 06:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]