On 10/19/2015 11:19 AM, Markus Armbruster wrote: > I'm not quite comfortable with reserving 'u' now, becaue I feel we > haven't fully explored the design space for avoiding branch - member > clashes. > > I still like the basic idea to give the unnamed union a name. It needs > to be a short one, to keep the C code legible. 'u' is an obvious > option, but it requires reserving 'u' at least as member name. '_u' > wouldn't. Alternatively, call the union 'u', but avoid the clash by > mapping QAPI member name 'u' to C identifier '_u'.
Naming the union '_u' is a bit uglier (more typing in every client)
whereas munging just the member name (what about 'q_u', the way c_name
appends 'q_' to any other name that would otherwise collide) pushes the
ugliness only to the C code that actually uses a member named 'u'.
But the idea of teaching c_name() to munge 'u' as a member name
certainly seems doable, at which point we no longer need to reserve 'u'
as a member name.
>
> I feel the decision should be made over the patch that give the union a
> name.
Well, that's patch 7/17 of this series, so at most, all I need to do is
shuffle things around when rebasing for v10.
For that matter, if we make c_name() munge 'u', it can just as easily
munge a member named 'has_' to 'q_has' (or 'base' to 'q_base' - except
that we are getting rid of that); or whatever other names we burn for
convenience on the C side.
But no one is using a member named 'u' at the moment, so it's not the
most critical problem to solve; and forbidding it is certainly
conservative (we can relax things to allow the name in QMP after all,
once we figure out the appropriate munging for the C side).
>> +++ b/scripts/qapi.py
>> @@ -488,6 +488,10 @@ def check_type(expr_info, source, value,
>> allow_array=False,
>> for (key, arg) in value.items():
>> check_name(expr_info, "Member of %s" % source, key,
>> allow_optional=allow_optional)
>> + if key == 'u' or key.startswith('has-') or key.startswith('has_'):
>
> Something like c_name(key).startswith('has_') would avoid hardcoding the
> mapping of '-' to '_' here. Dunno.
Oh, nice idea.
And looking at that, we have a number of places in qapi.py that are
using things like str[-4:] == '....' that might look nicer as
str.endwith('....'). I may add an obvious trivial cleanup patch into the
mix.
>> @@ -588,6 +592,14 @@ def check_union(expr, expr_info):
>> # Check every branch
>> for (key, value) in members.items():
>> check_name(expr_info, "Member of union '%s'" % name, key)
>> + # TODO: As long as branch names can collide with QMP names, we
>> + # must prevent branches starting with 'has_'. However, we do not
>> + # need to reject 'u', because that is reserved for when we start
>> + # sticking branch names in a C union named 'u'.
>> + if key.startswith('has-') or key.startswith('has_'):
>> + raise QAPIExprError(expr_info,
>> + "Branch of union '%s' uses reserved name
>> '%s'"
>> + % (name, key))
>
> This will go away again when we give the unnamed union a name.
>
> I feel we should punt all further clash detection until late in the
> cleanup work. It's merely nice to have (sane error message from
> generator instead of possibly confusing one from the C compiler,
> basically), and adding it now causes churn later on.
Okay, I can respin along those lines - if my work later in the series
removes a negative test added earlier in the series, then strip that
test from the series as a whole rather than fighting the churn, to
reduce the size of the series.
>> +++ b/tests/qapi-schema/args-name-has.json
>> @@ -1,6 +1,5 @@
>> # C member name collision
>> -# FIXME - This parses, but fails to compile, because the C struct is given
>> -# two 'has_a' members, one from the flag for optional 'a', and the other
>> -# from member 'has-a'. Either reject this at parse time, or munge the C
>> -# names to avoid the collision.
>> +# This would attempt to create two 'has_a' members of the C struct, one
>> +# from the flag for optional 'a', and the other from member 'has-a'.
>> +# TODO we could munge the optional flag name to avoid the collision.
>
> You mean call them _has_FOO instead of has_FOO? The generated code
> would be rather confusing...
>
> If we don't want to reserve all names starting with 'has_', then I'd
> narrowly outlaw having both an optional member FOO and a member has_FOO.
> I think I'd like that a bit better than outlawing 'has_'. But not
> enough to accept much implementation complexity.
The problem comes with child classes - we don't know a priori if an
optional member in one struct will end up being a base class to another
struct or union where the child class will hit the name clash. It's
easier to outlaw the name, or else come up with a munging scheme that
never clashes. Changing the existing has_ naming of flags is awkward
(lots of existing code) compared to munging the (unlikely) addition of a
new has_ member to a single qapi type.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
