diff mbox

[03/17] tests: remove alt num-int cases

Message ID 87tw4cn7sh.fsf@dusky.pond.sub.org
State New
Headers show

Commit Message

Markus Armbruster May 22, 2017, 5:03 p.m. UTC 
Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> There are no real users of this case, and it's going to be invalid after
> merging of QFloat and QInt use the same QNum type in the following patch.

Invalid because our alternate code is insufficiently sophisticated.  In
other words fixable.  See "[PATCH 4/4] qapi: Reject alternates that
can't work with keyval_parse()" I just posted.

My patch's commit message describes two related issues, one fixable and
one unfixable.  The fixable one already exists, but only in QGA.  I
intend to fix it, but it's not a priority.

Fixing the issue you describe seems even less important.  I considered
outlawing it in my series (patch appended for your reading pleasure),
but decided to leave it to yours.  I don't expect you to replace your
patch.  Perhaps my patch helps you with rebasing yours should that
become necessary.

Comments

Fam Zheng May 30, 2017, 3:40 a.m. UTC | #1 
I noticed you were wondering what happend to this message in the patchew thread:

http://patchew.org/QEMU/20170509173559.31598-1-marcandre.lureau@redhat.com/

Markus, apparently this is because of the unusual "In-Reply-To" header of your
message, which confuses patchew:

In-Reply-To: <20170509173559.31598-4-marcandre.lureau@redhat.com>
	(=?utf-8?Q?=22Marc-Andr=C3=A9?=
	Lureau"'s message of "Tue, 9 May 2017 20:35:45 +0300")

Is this an extension of your email client, or a standard?

In https://tools.ietf.org/html/rfc2822#section-3.6.4:
>    The "References:" and "In-Reply-To:" field each contain one or more
>    unique message identifiers, optionally separated by CFWS.
> 
>    The message identifier (msg-id) is similar in syntax to an angle-addr
>    construct without the internal CFWS.
> 
> message-id      =       "Message-ID:" msg-id CRLF
> 
> in-reply-to     =       "In-Reply-To:" 1*msg-id CRLF
> 
> references      =       "References:" 1*msg-id CRLF

I can imagine if consider CFWS, patchew should probably just work (I can fix
that), but it's still good to understand this.

Fam
Eric Blake May 30, 2017, 2:17 p.m. UTC | #2 
On 05/29/2017 10:40 PM, Fam Zheng wrote:

> In-Reply-To: <20170509173559.31598-4-marcandre.lureau@redhat.com>
> 	(=?utf-8?Q?=22Marc-Andr=C3=A9?=
> 	Lureau"'s message of "Tue, 9 May 2017 20:35:45 +0300")
> 

>> in-reply-to     =       "In-Reply-To:" 1*msg-id CRLF
>>

msg-id          =       [CFWS] "<" id-left "@" id-right ">" [CFWS]

then looking at 3.2.3 on folding white space and comments:

CFWS            =       *([FWS] comment) (([FWS] comment) / FWS)

comment         =       "(" *([FWS] ccontent) [FWS] ")"

> I can imagine if consider CFWS, patchew should probably just work (I can fix
> that), but it's still good to understand this.

So yes, I think the comment is valid per the grammar, and patchew should
be taught to recognize it.
diff mbox

Patch

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b7a25e4..06e583d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -826,6 +826,10 @@  def check_alternate(expr, info):
                 conflicting.add('QTYPE_QINT')
                 conflicting.add('QTYPE_QFLOAT')
                 conflicting.add('QTYPE_QBOOL')
+        elif qtype == 'QTYPE_QINT':
+            conflicting.add('QTYPE_QFLOAT')
+        elif qtype == 'QTYPE_QFLOAT':
+            conflicting.add('QTYPE_QINT')
         if conflicting & set(types_seen):
             raise QAPISemError(info, "Alternate '%s' member '%s' can't "
                                "be distinguished from member '%s'"
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 17649c6..91ffb26 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -103,8 +103,6 @@ 
 { 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
 { 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
 { 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
-{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
-{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
 
 # for testing use of 'str' within alternates
 { 'alternate': 'AltStrObj', 'data': { 's': 'str', 'o': 'TestStruct' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 9f68610..e727a5a 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -10,18 +10,10 @@  alternate AltEnumNum
     tag type
     case e: EnumOne
     case n: number
-alternate AltIntNum
-    tag type
-    case i: int
-    case n: number
 alternate AltNumEnum
     tag type
     case n: number
     case e: EnumOne
-alternate AltNumInt
-    tag type
-    case n: number
-    case i: int
 alternate AltStrObj
     tag type
     case s: str
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index c3be005..baf7e33 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -615,7 +615,7 @@  static void test_keyval_visit_alternate(void)
     Visitor *v;
     QDict *qdict;
     AltStrObj *aso;
-    AltNumInt *ani;
+    AltNumEnum *ane;
     AltEnumBool *aeb;
 
     /*
@@ -631,7 +631,7 @@  static void test_keyval_visit_alternate(void)
     g_assert_cmpint(aso->type, ==, QTYPE_QSTRING);
     g_assert_cmpstr(aso->u.s, ==, "1");
     qapi_free_AltStrObj(aso);
-    visit_type_AltNumInt(v, "b", &ani, &err);
+    visit_type_AltNumEnum(v, "b", &ane, &err);
     error_free_or_abort(&err);
     visit_type_AltEnumBool(v, "c", &aeb, &err);
     error_free_or_abort(&err);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 6b997a1..83d663d 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -592,8 +592,6 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     AltEnumNum *aen;
     AltNumEnum *ans;
     AltEnumInt *asi;
-    AltIntNum *ain;
-    AltNumInt *ani;
 
     /* Parsing an int */
 
@@ -620,18 +618,6 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     g_assert_cmpint(asi->u.i, ==, 42);
     qapi_free_AltEnumInt(asi);
 
-    v = visitor_input_test_init(data, "42");
-    visit_type_AltIntNum(v, NULL, &ain, &error_abort);
-    g_assert_cmpint(ain->type, ==, QTYPE_QINT);
-    g_assert_cmpint(ain->u.i, ==, 42);
-    qapi_free_AltIntNum(ain);
-
-    v = visitor_input_test_init(data, "42");
-    visit_type_AltNumInt(v, NULL, &ani, &error_abort);
-    g_assert_cmpint(ani->type, ==, QTYPE_QINT);
-    g_assert_cmpint(ani->u.i, ==, 42);
-    qapi_free_AltNumInt(ani);
-
     /* Parsing a double */
 
     v = visitor_input_test_init(data, "42.5");
@@ -655,18 +641,6 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     visit_type_AltEnumInt(v, NULL, &asi, &err);
     error_free_or_abort(&err);
     qapi_free_AltEnumInt(asi);
-
-    v = visitor_input_test_init(data, "42.5");
-    visit_type_AltIntNum(v, NULL, &ain, &error_abort);
-    g_assert_cmpint(ain->type, ==, QTYPE_QFLOAT);
-    g_assert_cmpfloat(ain->u.n, ==, 42.5);
-    qapi_free_AltIntNum(ain);
-
-    v = visitor_input_test_init(data, "42.5");
-    visit_type_AltNumInt(v, NULL, &ani, &error_abort);
-    g_assert_cmpint(ani->type, ==, QTYPE_QFLOAT);
-    g_assert_cmpfloat(ani->u.n, ==, 42.5);
-    qapi_free_AltNumInt(ani);
 }
 
 static void test_native_list_integer_helper(TestInputVisitorData *data,