Eric Blake <ebl...@redhat.com> writes:
> We have toyed on list with the idea of a future extension to
> QMP of teaching it to be case-insensitive (the user could
> request command 'Quit' instead of 'quit', or could spell a
> struct field as 'CPU' instead of 'cpu'). But for that to be
> a practical extension, we cannot break backwards compatibility
> with any existing struct that was already relying on case
> sensitivity. Fortunately, after the previous patch cleaned
> up CpuInfo, there are no such existing qapi structs.
>
> Another benefit of enforcing a restriction against
> case-insensitive is that we no longer have to worry about the
> case of enum values that could be distinguished by case if
> mapped by c_name(), but which cannot be distinguished when
> mapped to C as ALL_CAPS by camel_to_uppper(). Having the
camel_to_upper()
> generator look for case collisions up front will prevent
> developers from worrying whether different munging rules
> for member names compared to enum values as a discriminator
> will cause any problems in qapi unions.
>
> Of course, if we never implement a case-insensitive QMP
> extension, or find a legitimate reason to need qapi members
> that differ only by case, we could always relax this
> restriction. But it is easier to relax later than it is to
> wish we had the restriction in place earlier.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
I think "make QMP case-insensitive" is a very weak justification for
anything.
Your second reason is much stronger: forbidding case-insensitive clashes
lets the generators shout without fear of introducing clashes in
generated code.
However, since camel_to_upper() does more than just shout, forbidding
case-insensitive clashes protects against clashes in generated code only
for names where camel_to_upper() just shouts. It does for lower case
names, i.e. almost all of them.
The commit would make more sense if we first defanged c_enum_const():
make it mangle the const_name part like c_name(const_name).upper().
This is independent of the argument I'm having with Dan on how the
prefix should be mangled.
One more good argument for forbidding case-insensitive clashes: an
interface that sports names differing only in case is a bad interface.
> ---
> v10: new patch
> ---
> docs/qapi-code-gen.txt | 5 +++++
> scripts/qapi.py | 4 ++--
> tests/Makefile | 1 +
> tests/qapi-schema/args-case-clash.err | 1 +
> tests/qapi-schema/args-case-clash.exit | 1 +
> tests/qapi-schema/args-case-clash.json | 5 +++++
> tests/qapi-schema/args-case-clash.out | 0
> 7 files changed, 15 insertions(+), 2 deletions(-)
> create mode 100644 tests/qapi-schema/args-case-clash.err
> create mode 100644 tests/qapi-schema/args-case-clash.exit
> create mode 100644 tests/qapi-schema/args-case-clash.json
> create mode 100644 tests/qapi-schema/args-case-clash.out
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index f9fa6f3..13cec10 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -116,6 +116,11 @@ names should be ALL_CAPS with words separated by
> underscore. Field
> names cannot start with 'has-' or 'has_', as this is reserved for
> tracking optional fields.
>
> +For now, Client JSON Protocol is case-sensitive, but future extensions
> +may allow for case-insensitive recognition of command and event names,
> +or of member field names. As such, the generator rejects attempts to
> +create entities that only differ by case.
> +
This suggests we're actually planning to make QMP case-insensitive.
Let's avoid that. Perhaps:
Since we don't want interfaces with different names that differ only
in case, the generator rejects such names.
> Any name (command, event, type, field, or enum value) beginning with
> "x-" is marked experimental, and may be withdrawn or changed
> incompatibly in a future release. Downstream vendors may add
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3a359cb..7e7ad6e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1039,7 +1039,7 @@ class QAPISchemaObjectTypeMember(object):
> assert self.type
>
> def check_clash(self, info, seen):
> - name = c_name(self.name)
> + name = c_name(self.name).upper()
> if name in seen:
> raise QAPIExprError(info,
> "%s collides with %s"
> @@ -1087,7 +1087,7 @@ class QAPISchemaObjectTypeVariants(object):
>
> def check(self, schema, seen):
> if self.tag_name: # flat union
> - self.tag_member = seen[c_name(self.tag_name)]
> + self.tag_member = seen[c_name(self.tag_name).upper()]
> assert self.tag_name == self.tag_member.name
> tag_type = self.tag_member.type
> assert isinstance(tag_type, QAPISchemaEnumType)
If we ever acquire more lookups in seen, we'll need a lookup function,
so we don't duplicate c_name(NAME).upper(). But this will do for now.
> diff --git a/tests/Makefile b/tests/Makefile
> index c84c6cb..d1c6817 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -239,6 +239,7 @@ qapi-schema += args-alternate.json
> qapi-schema += args-any.json
> qapi-schema += args-array-empty.json
> qapi-schema += args-array-unknown.json
> +qapi-schema += args-case-clash.json
> qapi-schema += args-int.json
> qapi-schema += args-invalid.json
> qapi-schema += args-member-array-bad.json
> diff --git a/tests/qapi-schema/args-case-clash.err
> b/tests/qapi-schema/args-case-clash.err
> new file mode 100644
> index 0000000..5495ab8
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-case-clash.json:5: 'A' (argument of oops) collides
> with 'a' (argument of oops)
> diff --git a/tests/qapi-schema/args-case-clash.exit
> b/tests/qapi-schema/args-case-clash.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/args-case-clash.json
> b/tests/qapi-schema/args-case-clash.json
> new file mode 100644
> index 0000000..55ae488
> --- /dev/null
> +++ b/tests/qapi-schema/args-case-clash.json
> @@ -0,0 +1,5 @@
> +# C member name collision
> +# Reject members that clash case-insensitively (our mapping to C names
> +# preserves case, but allowing these members now would prevent a future
> +# relaxing of QMP to be case-insensitive).
Again, let's not suggest we're planning to make QMP case-insensitive.
> +{ 'command': 'oops', 'data': { 'a': 'str', 'A': 'int' } }
> diff --git a/tests/qapi-schema/args-case-clash.out
> b/tests/qapi-schema/args-case-clash.out
> new file mode 100644
> index 0000000..e69de29
