Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Am 11.11.2015 um 07:51 schrieb Eric Blake:
> When munging enum values, the fact that we were passing the entire
> prefix + value through camel_to_upper() meant that enum values
> spelled with CamelCase could be turned into CAMEL_CASE. However,
> this provides a potential collision (both OneTwo and One-Two would
> munge into ONE_TWO). By changing the generation of enum constants
> to always be prefix + '_' + c_name(value).upper(), we can avoid
> any risk of collisions (if we can also ensure no case collisions,
> in the next patch) without having to think about what the
> heuristics in camel_to_upper() will actually do to the value.
>
> Thankfully, only two enums are affected: ErrorClass and InputButton.
>
> Suggested by: Markus Armbruster
> Signed-off-by: Eric Blake
[...]
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index a35098f..1c39432 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -461,7 +461,7 @@ static BusState *qbus_find(const char *path, Error **errp)
> pos += len;
> dev = qbus_find_dev(bus, elem);
> if (!dev) {
> -error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +error_set(errp, ERROR_CLASS_DEVICENOTFOUND,
>"Device '%s' not found", elem);
> qbus_list_dev(bus, errp);
> return NULL;
> @@ -788,7 +788,7 @@ void qmp_device_del(const char *id, Error **errp)
> }
>
> if (!obj) {
> -error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +error_set(errp, ERROR_CLASS_DEVICENOTFOUND,
>"Device '%s' not found", id);
> return;
> }
[...]
> diff --git a/qom/object.c b/qom/object.c
> index c0decb6..425b7c3 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1018,7 +1018,7 @@ Object *object_property_get_link(Object *obj, const
> char *name,
> if (str && *str) {
> target = object_resolve_path(str, NULL);
> if (!target) {
> -error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +error_set(errp, ERROR_CLASS_DEVICENOTFOUND,
>"Device '%s' not found", str);
> }
> }
> @@ -1337,7 +1337,7 @@ static Object *object_resolve_link(Object *obj, const
> char *name,
> if (target || ambiguous) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
> } else {
> -error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +error_set(errp, ERROR_CLASS_DEVICENOTFOUND,
>"Device '%s' not found", path);
> }
> target = NULL;
That spelling is not exactly an improvement, but well,
Reviewed-by: Andreas Färber
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Eric Blake writes:
> When munging enum values, the fact that we were passing the entire
> prefix + value through camel_to_upper() meant that enum values
> spelled with CamelCase could be turned into CAMEL_CASE. However,
> this provides a potential collision (both OneTwo and One-Two would
> munge into ONE_TWO). By changing the generation of enum constants
> to always be prefix + '_' + c_name(value).upper(), we can avoid
> any risk of collisions (if we can also ensure no case collisions,
> in the next patch) without having to think about what the
> heuristics in camel_to_upper() will actually do to the value.
This is the good part: the rules for clashes become much simpler.
Bonus: the implementation for detecting them will be simple, too.
> Thankfully, only two enums are affected: ErrorClass and InputButton.
By convention (see CODING_STYLE), we use CamelCase for type names, and
nothing else.
Only enums violating this naming convention can be affected. The bad
part: they exist.
InputButton has two camels: WheelUp and WheelDown. The C enumeration
constants change from INPUT_BUTTON_WHEEL_UP/WHEEL_DOWN to
INPUT_BUTTON_WHEELUP/WHEELDOWN. Not exactly an improvement, but one,
there are just 21 occurences in 11 files, and two, I think we can still
fix the enumeration to "lower case with dash", as it's only used by
x-input-send-event.
ErrorClass's members are all camels. The C enumeration constants change
as follows
ERROR_CLASS_GENERIC_ERROR ERROR_CLASS_GENERICERROR
ERROR_CLASS_COMMAND_NOT_FOUND ERROR_CLASS_COMMANDNOTFOUND
ERROR_CLASS_DEVICE_ENCRYPTEDERROR_CLASS_DEVICEENCRYPTED
ERROR_CLASS_DEVICE_NOT_ACTIVE ERROR_CLASS_DEVICENOTACTIVE
ERROR_CLASS_DEVICE_NOT_FOUNDERROR_CLASS_DEVICENOTFOUND
ERROR_CLASS_KVM_MISSING_CAP ERROR_CLASS_KVMMISSINGCAP
Again, not an improvement, but perhaps tolerabe, because these constants
aren't used much anymore: 55 occurences in 20 files.
If we find it not tolerable, we can manually add aliases: rename the
QAPI type out of the way, say 'QAPIErrorClass', then stick
typedef enum ErrorClass {
ERROR_CLASS_GENERIC_ERROR = QAPI_ERROR_CLASS_GENERICERROR,
ERROR_CLASS_COMMAND_NOT_FOUND = QAPI_ERROR_CLASS_COMMANDNOTFOUND,
ERROR_CLASS_DEVICE_ENCRYPTED = QAPI_ERROR_CLASS_DEVICEENCRYPTED,
ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICENOTACTIVE,
ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICENOTFOUND,
ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVMMISSINGCAP,
} ErrorClass;
into error.h with a suitable comment.
Opinions?
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
[hmm, wonder why scripts/get-maintainer.pl didn't loop in Gerd to the patch itself] On 11/11/2015 07:50 AM, Markus Armbruster wrote: > Eric Blake writes: > >> When munging enum values, the fact that we were passing the entire >> prefix + value through camel_to_upper() meant that enum values >> spelled with CamelCase could be turned into CAMEL_CASE. However, >> this provides a potential collision (both OneTwo and One-Two would >> munge into ONE_TWO). By changing the generation of enum constants >> to always be prefix + '_' + c_name(value).upper(), we can avoid >> any risk of collisions (if we can also ensure no case collisions, >> in the next patch) without having to think about what the >> heuristics in camel_to_upper() will actually do to the value. > > This is the good part: the rules for clashes become much simpler. > > Bonus: the implementation for detecting them will be simple, too. > >> Thankfully, only two enums are affected: ErrorClass and InputButton. Visiting just InputButton in this email. > > By convention (see CODING_STYLE), we use CamelCase for type names, and > nothing else. > > Only enums violating this naming convention can be affected. The bad > part: they exist. > > InputButton has two camels: WheelUp and WheelDown. The C enumeration > constants change from INPUT_BUTTON_WHEEL_UP/WHEEL_DOWN to > INPUT_BUTTON_WHEELUP/WHEELDOWN. Not exactly an improvement, but one, > there are just 21 occurences in 11 files, and two, I think we can still > fix the enumeration to "lower case with dash", as it's only used by > x-input-send-event. The InputButton type has existed since 2.0; which is then part of the 'InputBtnEvent' struct, then the 'InputEvent' union, also since 2.0. I can't easily tell if it was only used internally at that point, or if we exposed it through the command line (even if we didn't expose it through QMP); but I concur with your reading that in QMP it is only used via 'x-input-send-event' (since 2.2, but the x- prefix gives us freedom). [Oh, and I just spotted a typo; 'InputAxis' has a copy-and-paste doc typo calling it 'InputButton'] If desired, I can prepare an alternate patch that adds the dash to the qapi enum definition, to see what we think. But meanwhile, look at some of the lines in the patch: > diff --git a/ui/sdl.c b/ui/sdl.c > index 570cb99..2678611 100644 > --- a/ui/sdl.c > +++ b/ui/sdl.c > @@ -469,8 +469,8 @@ static void sdl_send_mouse_event(int dx, int dy, int x, > int y, int state) > [INPUT_BUTTON_LEFT] = SDL_BUTTON(SDL_BUTTON_LEFT), > [INPUT_BUTTON_MIDDLE] = SDL_BUTTON(SDL_BUTTON_MIDDLE), > [INPUT_BUTTON_RIGHT] = SDL_BUTTON(SDL_BUTTON_RIGHT), > -[INPUT_BUTTON_WHEEL_UP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), > -[INPUT_BUTTON_WHEEL_DOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), > +[INPUT_BUTTON_WHEELUP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), > +[INPUT_BUTTON_WHEELDOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), Since SDL already spells the names without space, it's not the end of the world if we do likewise. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
On 11/11/2015 07:50 AM, Markus Armbruster wrote:
>> Thankfully, only two enums are affected: ErrorClass and InputButton.
[Visiting just ErrorClass in this email]
>
> By convention (see CODING_STYLE), we use CamelCase for type names, and
> nothing else.
>
> Only enums violating this naming convention can be affected. The bad
> part: they exist.
>
>
> ErrorClass's members are all camels. The C enumeration constants change
> as follows
>
> ERROR_CLASS_GENERIC_ERROR ERROR_CLASS_GENERICERROR
> ERROR_CLASS_COMMAND_NOT_FOUND ERROR_CLASS_COMMANDNOTFOUND
> ERROR_CLASS_DEVICE_ENCRYPTEDERROR_CLASS_DEVICEENCRYPTED
> ERROR_CLASS_DEVICE_NOT_ACTIVE ERROR_CLASS_DEVICENOTACTIVE
> ERROR_CLASS_DEVICE_NOT_FOUNDERROR_CLASS_DEVICENOTFOUND
> ERROR_CLASS_KVM_MISSING_CAP ERROR_CLASS_KVMMISSINGCAP
>
> Again, not an improvement, but perhaps tolerabe, because these constants
> aren't used much anymore: 55 occurences in 20 files.
>
> If we find it not tolerable, we can manually add aliases: rename the
> QAPI type out of the way, say 'QAPIErrorClass', then stick
>
> typedef enum ErrorClass {
> ERROR_CLASS_GENERIC_ERROR = QAPI_ERROR_CLASS_GENERICERROR,
> ERROR_CLASS_COMMAND_NOT_FOUND = QAPI_ERROR_CLASS_COMMANDNOTFOUND,
> ERROR_CLASS_DEVICE_ENCRYPTED = QAPI_ERROR_CLASS_DEVICEENCRYPTED,
> ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICENOTACTIVE,
> ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICENOTFOUND,
> ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVMMISSINGCAP,
> } ErrorClass;
>
> into error.h with a suitable comment.
Interesting hack. Certainly more legible, and I'm willing to attempt
such a rename.
As far as I'm concerned, though, this does not fix a bug, so much as
make it easier for later qapi patches to be more robust. So any
renaming we do is not under pressure to have to make it in by 2.5.
We've got time to think about it.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Eric Blake writes: > [hmm, wonder why scripts/get-maintainer.pl didn't loop in Gerd to the > patch itself] > > On 11/11/2015 07:50 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> When munging enum values, the fact that we were passing the entire >>> prefix + value through camel_to_upper() meant that enum values >>> spelled with CamelCase could be turned into CAMEL_CASE. However, >>> this provides a potential collision (both OneTwo and One-Two would >>> munge into ONE_TWO). By changing the generation of enum constants >>> to always be prefix + '_' + c_name(value).upper(), we can avoid >>> any risk of collisions (if we can also ensure no case collisions, >>> in the next patch) without having to think about what the >>> heuristics in camel_to_upper() will actually do to the value. >> >> This is the good part: the rules for clashes become much simpler. >> >> Bonus: the implementation for detecting them will be simple, too. >> >>> Thankfully, only two enums are affected: ErrorClass and InputButton. > > Visiting just InputButton in this email. > >> >> By convention (see CODING_STYLE), we use CamelCase for type names, and >> nothing else. >> >> Only enums violating this naming convention can be affected. The bad >> part: they exist. >> >> InputButton has two camels: WheelUp and WheelDown. The C enumeration >> constants change from INPUT_BUTTON_WHEEL_UP/WHEEL_DOWN to >> INPUT_BUTTON_WHEELUP/WHEELDOWN. Not exactly an improvement, but one, >> there are just 21 occurences in 11 files, and two, I think we can still >> fix the enumeration to "lower case with dash", as it's only used by >> x-input-send-event. > > The InputButton type has existed since 2.0; which is then part of the > 'InputBtnEvent' struct, then the 'InputEvent' union, also since 2.0. I > can't easily tell if it was only used internally at that point, or if we > exposed it through the command line (even if we didn't expose it through > QMP); As far as I can tell, we used it internally, and for tracing. > but I concur with your reading that in QMP it is only used via > 'x-input-send-event' (since 2.2, but the x- prefix gives us freedom). > [Oh, and I just spotted a typo; 'InputAxis' has a copy-and-paste doc > typo calling it 'InputButton'] > > If desired, I can prepare an alternate patch that adds the dash to the > qapi enum definition, to see what we think. If Gerd is fine with the rename, let's do it. > But meanwhile, look at some of the lines in the patch: > >> diff --git a/ui/sdl.c b/ui/sdl.c >> index 570cb99..2678611 100644 >> --- a/ui/sdl.c >> +++ b/ui/sdl.c >> @@ -469,8 +469,8 @@ static void sdl_send_mouse_event(int dx, int dy, >> int x, int y, int state) >> [INPUT_BUTTON_LEFT] = SDL_BUTTON(SDL_BUTTON_LEFT), >> [INPUT_BUTTON_MIDDLE] = SDL_BUTTON(SDL_BUTTON_MIDDLE), >> [INPUT_BUTTON_RIGHT] = SDL_BUTTON(SDL_BUTTON_RIGHT), >> -[INPUT_BUTTON_WHEEL_UP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), >> -[INPUT_BUTTON_WHEEL_DOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), >> +[INPUT_BUTTON_WHEELUP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), >> +[INPUT_BUTTON_WHEELDOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), > > Since SDL already spells the names without space, it's not the end of > the world if we do likewise. Good point. Even if we adopt SDL's spelling WHEELUP and WHEELDOWN, I'd still prefer to downcase the QAPI names for consistency with the rest of QAPI.
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Hi, > The InputButton type has existed since 2.0; which is then part of the > 'InputBtnEvent' struct, then the 'InputEvent' union, also since 2.0. I > can't easily tell if it was only used internally at that point, Internal only. > 'x-input-send-event' (since 2.2, but the x- prefix gives us freedom). Yes, x-input-send-event is the only external usage. cheers, Gerd
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Hi, > > If desired, I can prepare an alternate patch that adds the dash to the > > qapi enum definition, to see what we think. > > If Gerd is fine with the rename, let's do it. No need to do so I think ... > >> -[INPUT_BUTTON_WHEEL_UP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), > >> -[INPUT_BUTTON_WHEEL_DOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), > >> +[INPUT_BUTTON_WHEELUP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), > >> +[INPUT_BUTTON_WHEELDOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), > > > > Since SDL already spells the names without space, it's not the end of > > the world if we do likewise. > > Good point. This doesn't look too bad. And even if x-input-send-event isn't official api I'd prefer to not break it for such a minor cosmetic issue. cheers, Gerd
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Gerd Hoffmann writes: > Hi, > >> > If desired, I can prepare an alternate patch that adds the dash to the >> > qapi enum definition, to see what we think. >> >> If Gerd is fine with the rename, let's do it. > > No need to do so I think ... > >> >> -[INPUT_BUTTON_WHEEL_UP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), >> >> -[INPUT_BUTTON_WHEEL_DOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), >> >> +[INPUT_BUTTON_WHEELUP] = SDL_BUTTON(SDL_BUTTON_WHEELUP), >> >> +[INPUT_BUTTON_WHEELDOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN), >> > >> > Since SDL already spells the names without space, it's not the end of >> > the world if we do likewise. >> >> Good point. >> >> Even if we adopt SDL's spelling WHEELUP and WHEELDOWN, I'd still prefer >> to downcase the QAPI names for consistency with the rest of QAPI. > > This doesn't look too bad. And even if x-input-send-event isn't > official api I'd prefer to not break it for such a minor cosmetic issue. To slow our slide into a morass of inconsistency, I intend to make qapi.py enforce naming conventions. Involves a whitelist for existing violators we can't or won't fix. Naturally, I'd prefer to keep the list as short as possible. I feel these ones can and should be fixed, and the best time to fix them is when we drop the x- from the command. But if you insist on keeping the current names then, I'll live with the extra whitelist entries.
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
On 11/10/2015 11:51 PM, Eric Blake wrote:
> When munging enum values, the fact that we were passing the entire
> prefix + value through camel_to_upper() meant that enum values
> spelled with CamelCase could be turned into CAMEL_CASE. However,
> this provides a potential collision (both OneTwo and One-Two would
> munge into ONE_TWO). By changing the generation of enum constants
> to always be prefix + '_' + c_name(value).upper(), we can avoid
> any risk of collisions (if we can also ensure no case collisions,
> in the next patch) without having to think about what the
> heuristics in camel_to_upper() will actually do to the value.
>
> +++ b/scripts/qapi.py
> @@ -1439,7 +1439,7 @@ def camel_to_upper(value):
> def c_enum_const(type_name, const_name, prefix=None):
> if prefix is not None:
> type_name = prefix
> -return camel_to_upper(type_name + '_' + const_name)
> +return camel_to_upper(type_name) + '_' + c_name(const_name,
> False).upper()
Doesn't match the commit message, because it used c_name(,False), while
c_name(name) is short for c_name(name, True).
What's more, looking at it exposed a bug: c_name('_Thread-local')
returns '_Thread_local', but this collides with a C11 keyword (and was
supposed to be munged to q__Thread_local); add '_Thread-local':'int' to
a struct to see the resulting hilarity:
CCtests/test-qmp-output-visitor.o
In file included from tests/test-qmp-output-visitor.c:17:0:
tests/test-qapi-types.h:781:13: error: expected identifier or ‘(’ before
‘_Thread_local’
int64_t _Thread_local;
^
But it also made me realize that c_name('Q-int') happily returns 'Q_int'
(we only reserved the leading 'q_' namespace, not 'Q_'). Alas, that
means c_name('Q-int', False).upper() and c_name('int', False).upper()
both produce 'Q_INT', and we have a collision. So I think enum names
have to be munged by c_name(name, True).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Eric Blake writes:
> On 11/10/2015 11:51 PM, Eric Blake wrote:
>> When munging enum values, the fact that we were passing the entire
>> prefix + value through camel_to_upper() meant that enum values
>> spelled with CamelCase could be turned into CAMEL_CASE. However,
>> this provides a potential collision (both OneTwo and One-Two would
>> munge into ONE_TWO). By changing the generation of enum constants
>> to always be prefix + '_' + c_name(value).upper(), we can avoid
>> any risk of collisions (if we can also ensure no case collisions,
>> in the next patch) without having to think about what the
>> heuristics in camel_to_upper() will actually do to the value.
>>
>
>> +++ b/scripts/qapi.py
>> @@ -1439,7 +1439,7 @@ def camel_to_upper(value):
>> def c_enum_const(type_name, const_name, prefix=None):
>> if prefix is not None:
>> type_name = prefix
>> -return camel_to_upper(type_name + '_' + const_name)
>> +return camel_to_upper(type_name) + '_' + c_name(const_name,
>> False).upper()
>
> Doesn't match the commit message, because it used c_name(,False), while
> c_name(name) is short for c_name(name, True).
>
> What's more, looking at it exposed a bug: c_name('_Thread-local')
> returns '_Thread_local', but this collides with a C11 keyword (and was
> supposed to be munged to q__Thread_local); add '_Thread-local':'int' to
> a struct to see the resulting hilarity:
>
> CCtests/test-qmp-output-visitor.o
> In file included from tests/test-qmp-output-visitor.c:17:0:
> tests/test-qapi-types.h:781:13: error: expected identifier or ‘(’ before
> ‘_Thread_local’
> int64_t _Thread_local;
> ^
c_name() first protects ticklish identifiers, then mangles. That's
exactly backwards.
> But it also made me realize that c_name('Q-int') happily returns 'Q_int'
> (we only reserved the leading 'q_' namespace, not 'Q_'). Alas, that
> means c_name('Q-int', False).upper() and c_name('int', False).upper()
> both produce 'Q_INT', and we have a collision. So I think enum names
> have to be munged by c_name(name, True).
Our goal is to have simple rules for reserved names and collisions, and
resonably simple code to catch them.
"[PATCH 20] qapi: Forbid case-insensitive clashes" is incomplete --- if
we make clashing case-insensitive, we need to reserve names
case-insensitively, too.
We need c_name() to protect ticklish identifiers only when its result is
used as identifier. Not when it's *part* of an identifier,
e.g. prefixed with qapi_, or camel_to_upper(type_name) + '_'.
We can protect even when we don't need to, if that helps keeping things
simple.
The obvious simple way to check for collisions works like this:
1. Every QAPI name is mangled in exactly one way, modulo case: always
with c_name(), and always with the same value of protect.
2. We require the mangled name to be case-insensitively unique in its
name space.
Any holes left?
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
On 11/13/2015 11:13 AM, Markus Armbruster wrote: > We need c_name() to protect ticklish identifiers only when its result is > used as identifier. Not when it's *part* of an identifier, > e.g. prefixed with qapi_, or camel_to_upper(type_name) + '_'. > > We can protect even when we don't need to, if that helps keeping things > simple. As far as I can tell, as soon as we reserve Q_ in addition to q_, and fix the bug in c_name() applying munging backwards, then we are safe to state that any comparison between two names for collisions will be accurate as long as both sides of the comparison picked the same value of c_name(name, protect), whether or not our later use of that name is done with protect=True or protect=False. > > The obvious simple way to check for collisions works like this: > > 1. Every QAPI name is mangled in exactly one way, modulo case: always >with c_name(), and always with the same value of protect. That part is easy. Maybe I could even reuse guardname(name) instead of c_name(name).upper(), although I don't know that it buys any clarity. > > 2. We require the mangled name to be case-insensitively unique in its >name space. That part is a bit harder: we unfortunately have the existing clash between the command 'stop' and the event 'STOP'. I didn't find any other clashes in existing clients, though (phew), so we can either whitelist just that one, or more likely, set up separate namespaces for commands vs. events when doing the case collision tests. But it's a bummer that it won't be quite as easy as I had hoped (using a single dictionary for both lookups and case collisions is easier than having two dictionaries to separate namespaces, while still searching both dictionaries for lookups). At any rate, I'm playing with patches along these lines. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values
Eric Blake writes: > On 11/13/2015 11:13 AM, Markus Armbruster wrote: > >> We need c_name() to protect ticklish identifiers only when its result is >> used as identifier. Not when it's *part* of an identifier, >> e.g. prefixed with qapi_, or camel_to_upper(type_name) + '_'. >> >> We can protect even when we don't need to, if that helps keeping things >> simple. > > As far as I can tell, as soon as we reserve Q_ in addition to q_, and > fix the bug in c_name() applying munging backwards, then we are safe to > state that any comparison between two names for collisions will be > accurate as long as both sides of the comparison picked the same value > of c_name(name, protect), whether or not our later use of that name is > done with protect=True or protect=False. Okay. >> The obvious simple way to check for collisions works like this: >> >> 1. Every QAPI name is mangled in exactly one way, modulo case: always >>with c_name(), and always with the same value of protect. > > That part is easy. Maybe I could even reuse guardname(name) instead of > c_name(name).upper(), although I don't know that it buys any clarity. Certainly not without a rename :) >> 2. We require the mangled name to be case-insensitively unique in its >>name space. > > That part is a bit harder: we unfortunately have the existing clash > between the command 'stop' and the event 'STOP'. I didn't find any > other clashes in existing clients, though (phew), so we can either > whitelist just that one, or more likely, set up separate namespaces for > commands vs. events when doing the case collision tests. I think the natural split is three: commands, events, types. > But it's a bummer that it won't be quite as easy as I had hoped (using a > single dictionary for both lookups and case collisions is easier than > having two dictionaries to separate namespaces, while still searching > both dictionaries for lookups). > > At any rate, I'm playing with patches along these lines. Before I dive into yet another lengthy treatise: I strongly recommend to kick clash detection work as far back in the queue as practical. It's hairy, and improving it isn't a priority. It has clogged the queue more than enough already. Let's take a step back and consider requirements. I think there are broadly two: the QAPI schema must make sense, and generation of clashing C identifiers should be caught. QAPI's requirements are pretty simple. It's always clear whether a name denotes a command, event, type, or member of a certain type. Thus, requiring names to be unique within their kind suffices. In other words, separate name spaces for commands, events, types and for each type's members. To avoid confusion, we can require more. Right now, we require command, event and type names to be unique within their combined kinds. In other words, a combined name space for commands, events and types. Catching C clashes is more complicated, because we need to analyze the generators' (less than disciplined) use of C's name spaces to find its requirements. QAPI names get mangled and combined with prefixes or suffixes to become C identifiers. Prefixes and suffixes can only reduce clashes among generated names (they can require reserving names, but let's ignore that here). Therefore, considering clashes between mangled QAPI names suffices. If we mangled each name in exactly one way, this would be trivial: use the mangled name as dictionary key, and use a separate dictionary for each C name space. Note it's trivial even with multiple ways to mangle, as long as each name gets mangled in exactly one of them. Unfortunately, this is not the case. We mangle in the following ways: * Standard mangling with c_name() Variations: protect=False, shouted, unshouted. * camel_to_upper() mangling Names that get mangled in more than one way, to the best of my knowledge: (1) Enumeration member names - Shouted standard mangling for the enumeration constant - Standard mangling for the union member when used as variant tag (2) Enumeration type names - Standard mangling for the typedef - camel_to_upper() mangling for the enumeration constants (3) Event names - Unshouted standard mangling for the function to emit the event - Shouted standard mangling for the enumeration constants (4) Possibly built-in type names - Standard mangling with protect=False - If we screw up somehow, standard mangling without protect=False General solution: put all possible manglings into the name space's dictionary. For example, enter a command name in standard mangling, but enter an event name twice, once in unshouted standard mangling, and once in shouted standard mangling. If we peek into a few black boxes, we can simplify things a bit. We use C's ordinary identifier name space, the struct tag name space, and the member name spaces of structs and unions we generate. We don't need dictionaries for the C struct, union and enum tag name spaces, because we always u
