Re: [Qemu-devel] [PATCH 3/5] qapi: avoid reserved word restrict

[Michael Roth]

On Tue, Jul 31, 2012 at 08:38:45PM +0000, Blue Swirl wrote:
> On Tue, Jul 31, 2012 at 6:55 PM, Michael Roth <address@hidden> wrote:
> > On Tue, Jul 31, 2012 at 04:56:50PM +0000, Blue Swirl wrote:
> >> On Tue, Jul 31, 2012 at 12:58 PM, Luiz Capitulino
> >> <address@hidden> wrote:
> >> > On Tue, 31 Jul 2012 09:28:43 +0200
> >> > Paolo Bonzini <address@hidden> wrote:
> >> >
> >> >> Il 30/07/2012 18:04, address@hidden ha scritto:
> >> >> > From: Blue Swirl <address@hidden>
> >> >> >
> >> >> > Clang compiler complained about use of reserved word 'restrict' in 
> >> >> > SLIRP
> >> >> > and QAPI.
> >> >> >
> >> >> > Rename 'restrict' to 'restricted' which also matches other SLIRP code.
> >> >>
> >> >> Can't do it, this changes the command-line option.
> >> >>
> >> >> Luiz, Michael, any ideas?
> >> >
> >> > I'm not sure how complicated it would be to implement this, but we could 
> >> > add
> >> > a 'bind' keyword to the type dict to control mapping between protocol 
> >> > names
> >> > and generated variable names. Like this:
> >> >
> >> > { 'type': 'NetdevUserOptions',
> >> >   'data': {
> >> >     '*hostname':  'str',
> >> >     '*restrict':  'bool',
> >> >     ...
> >> >     '*hostfwd':   ['String'],
> >> >     '*guestfwd':  ['String'] },
> >> >
> >> >   'bind': { 'restrict': 'restricted' } }
> >>
> >> How about prefixing all json-generated field names with for example
> >> 'json_'? Should be a simple mechanical change.
> >
> > It's a whole lot of churn though, and clobbers the history for most QMP
> > functions. It also seems like a strange thing for clang to complain about...
> 
> Not really, it's just C99:
> http://en.wikipedia.org/wiki/Restrict

As a struct field it didn't seem like there was a case where the usage
would be ambiguous, but yah, it's still justified to complain.

> 
> Prefixing would solve also future problems: 'if', 'auto', maybe
> 'static' can make sense for network options (as compared to DHCP) one
> day etc.
> 

I don't think there's a necessary risk of this happening again. We can
catch it in code review, warn against it in documentation, and even go as
far as adding a list of reserved keywords that the schema parser/code
generators can complain about.

(I'm also somewhat biased because QIDL needs the schema names to align with the
actual field names, since the schema in that case is a description of an
exiting type that is not generated by QAPI. Although it wouldn't be too
hard to add a command-line option to still support that...)

Another option I'll put out there is to have the code generators
special-case `field_name in [ <reserved keywords> ]:` to re-name the
fields internally, and document the behavior for developers. This keeps
our mistakes from spilling out into the QAPI schemas.

> >
> > I think special casing is probably the way to go...
> >
> > Luiz's bind approach seems reasonable, though "alias" might be the more
> > familiar name.
> >
> > As an alternative I'll throw out there, the QIDL series introduces the
> > notion of "annotated" fields, which allow us to pass additional
> > information to the code generators (instead of just the typename)
> > regarding how to handle that field internally. So we could do something 
> > like:
> >
> > { 'type': 'NetdevUserOptions',
> >   'data': {
> >     '*hostname':  'str',
> >     '*restrict': {
> >       '<annotated>': 'true',
> >       'type': 'bool',
> >       'native_name': 'restrict' },
> >     ...
> >     '*hostfwd':   ['String'],
> >     '*guestfwd':  ['String'] } }
> >
> > It's pretty flexible, but I'm hesitant to expose in documented schemas.
> 
> This could work too.
> 
> We could also introduce new names and deprecate old ones, one day we
> could remove the old versions. This is bit drastic change to be done
> just because a new user invisible implementation makes command line
> names clash with C keywords. I'd rather prefix.
> 
> >
> >>
> >> In addition to 'restrict', there may also be problems with 'if'
> >> (-drive, HMP drive_add) and maybe also 'auto' as value (several
> >> command line options, HMP pci_add) in the future.
> >>
>