qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' membe


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names
Date: Wed, 21 Oct 2015 15:08:15 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

> 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.

Munging QAPI name 'u' to C identifier 'q_u' begs the question what to do
with QAPI names 'q-u' and 'q_u'.

A relatively simple way out is to reserve QAPI names starting with 'q-'
and 'q_'.

I feel we need rules on name space and name munging instead of the
undisciplined "crap, got a collision, let me hack around it" approach
that's evident in the code.

Currently documented rules (docs/qapi-code-gen.txt):

1. Types, commands, and events share a common namespace.

2. Type definitions should always use CamelCase for user-defined type
   names, while built-in types are lowercase.

3. Type definitions should not end in 'Kind', as this namespace is used
   for creating implicit C enums for visiting union types.

4. Command names, and field names within a type, should be all lower
   case with words separated by a hyphen.  However, some existing older
   commands and complex types use underscore; when extending such
   expressions, consistency is preferred over blindly avoiding
   underscore.

5. Event names should be ALL_CAPS with words separated by underscore.

6. Other than downstream extensions (with leading underscore and the use
   of dots), all names should begin with a letter, and contain only
   ASCII letters, digits, dash, and underscore.

7. It is okay to reuse names that match C keywords; the generator will
   rename a field named "default" in the QAPI to "q_default" in the
   generated C code.

Note the non-use of "shall" for hard rules and "should" for mere
conventions.  As long as that's the case, a rule is hard when it's
enforced by a generator.

Rule 7 leaves "C keywords" unspecified.  If you think "C keywords" means
the keywords in the sense of the C standard, you're in for a surprise.

Rule 7 tacitly reserves "q_" + keyword, but an insufficiently paranoid
reader won't notice.

Rule 6 leaves C identifiers starting with underscore available for the
implementation.  However, C reserves all those for use as identifiers
with file scope, and the ones starting with two underscores or
underscore and uppercase latter for any use.

We can use identifiers starting with underscore and lower case letter or
digit for things like struct and union members, function parameters,
local variables.

If we need more, we'll have to reserve a suitable corner of the QAPI
namespace to make a some C namespace available for the implementation.
Reserving prefixes matching [qQ][-_] seems acceptable to me.

A thorough survey on how the generated C pollutes^Wmakes us of the C
namespaces would be in order, but I don't have time for it today.

>> 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).

I agree we need progress, not perfection.  When we accept an imperfect
solution to make progress, we should write down the issues for later,
though.  TODO comments should do.

>>> +++ 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.

Yes, .endswith() would be clearer.

>>> @@ -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.

Thanks.  I hope that'll help us focus and make progress.

>>> +++ 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.

Points to be considered in the context of the patch that actually does
something about the 'has-' collisions.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]