On 10/01/2015 05:51 AM, Markus Armbruster wrote: >> +++ b/tests/qapi-schema/alternate-clash.json >> @@ -1,3 +1,8 @@ >> -# we detect C enum collisions in an alternate >> +# Alternate branch name collision >> +# Reject an alternate that would result in a collision in generated C >> +# names (this would try to generate two enum values 'ALT1_KIND_A_B'). >> +# TODO: In the future, if alternates are simplified to not generate >> +# the implicit Alt1Kind enum, we would still have a collision with the >> +# resulting C union trying to have two members named 'a_b'. >> { 'alternate': 'Alt1', >> - 'data': { 'one': 'str', 'ONE': 'int' } } >> + 'data': { 'a-b': 'str', 'a_b': 'int' } } > > Ah, you're making the test slightly more robust. Works for me.
I just noticed we lack coverage for: { 'alternate': 'Alt', 'data': { 'type': 'int', 'other': 'str' } } (tries to create a C struct with two members named 'type', even after my v5 patches change to a simpler 'qtype_code type'). Can be done in my later patches that simplify alternates if we don't want a v8 spin of this part of the series. >> +++ b/tests/qapi-schema/flat-union-clash-type.json >> @@ -0,0 +1,15 @@ >> +# Flat union branch 'type' >> +# FIXME: this triggers an assertion failure. But even with that fixed, we >> +# would have a clash in generated C, between the outer tag 'type' and > > "outer tag"? I guess you mean the member type we inherit from Base. Yes. >> +++ b/tests/qapi-schema/flat-union-cycle.json >> @@ -0,0 +1,8 @@ >> +# Ensure that we have a sane error message for attempts at self-inheritance >> +# This test currently fails because we don't permit a union base, but >> +# even if we relax things to allow that in the future (see >> +# flat-union-base-union), self-inheritance should still fail. > > Do we have a test for the simpler case of a struct inheriting from > itself? Not here, but in v5 16/46. That's because it asserts, but while it was easy to fix up in the QAPISchema.check(), I did not find it worth the churn to fix it up in the ad hoc parse code just to rip it back out later, and the QAPISchema.check() code requires several scaffolding patches (so it wasn't as easy as fixing the union 'type' clash asserts). Tracking an assertion failure for any more than one patch at a time is horrible (as any other change to qapi.py changes line numbers that affect the assertion failure). > > I believe us merging struct and union types into a single object type is > more likely than us implementing union bases. If I'm correct, we won't > need this test. Maybe, but even then, we still have to decide if a base object can have variants. > > Found nothing that couldn't be touched up on commit. Your suggestions for wording tweaks are fine; I'm okay if you want to tweak it for your pull request instead of asking me for a v8. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature