diff mbox series

[v5,01/12] qapi: Move GenericList to qapi/util.h

Message ID 20201023183652.478921-2-eblake@redhat.com
State New
Headers show
Series Exposing backing-chain allocation over NBD | expand

Commit Message

Eric Blake Oct. 23, 2020, 6:36 p.m. UTC
Placing GenericList in util.h will make it easier for the next patch
to promote QAPI_LIST_ADD() into a public macro without requiring more
files to include the unrelated visitor.h.

However, we can't also move GenericAlternate; this is because it would
introduce a circular dependency: qapi-builtin-types.h needs a complete
definition of QEnumLookup (so it includes qapi/util.h), and
GenericAlternate needs a complete definition of QType (declared in
qapi-builtin-types.h).  Leaving GenericAlternate in visitor.h breaks
the cycle, and doesn't matter since we don't have any further planned
uses for that type outside of visitors.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 9 +--------
 include/qapi/util.h    | 8 ++++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Oct. 24, 2020, 9:06 a.m. UTC | #1
23.10.2020 21:36, Eric Blake wrote:
> Placing GenericList in util.h will make it easier for the next patch

> to promote QAPI_LIST_ADD() into a public macro without requiring more

> files to include the unrelated visitor.h.

> 

> However, we can't also move GenericAlternate; this is because it would

> introduce a circular dependency: qapi-builtin-types.h needs a complete

> definition of QEnumLookup (so it includes qapi/util.h), and

> GenericAlternate needs a complete definition of QType (declared in

> qapi-builtin-types.h).  Leaving GenericAlternate in visitor.h breaks

> the cycle, and doesn't matter since we don't have any further planned

> uses for that type outside of visitors.

> 

> Suggested-by: Markus Armbruster <armbru@redhat.com>

> Signed-off-by: Eric Blake <eblake@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


> ---

>   include/qapi/visitor.h | 9 +--------

>   include/qapi/util.h    | 8 ++++++++

>   2 files changed, 9 insertions(+), 8 deletions(-)

> 

> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h

> index ebc19ede7fff..8c2e1c29ad8b 100644

> --- a/include/qapi/visitor.h

> +++ b/include/qapi/visitor.h

> @@ -16,6 +16,7 @@

>   #define QAPI_VISITOR_H

> 

>   #include "qapi/qapi-builtin-types.h"

> +#include "qapi/util.h"

> 

>   /*

>    * The QAPI schema defines both a set of C data types, and a QMP wire

> @@ -228,14 +229,6 @@

> 

>   /*** Useful types ***/

> 

> -/* This struct is layout-compatible with all other *List structs

> - * created by the QAPI generator.  It is used as a typical

> - * singly-linked list. */

> -typedef struct GenericList {

> -    struct GenericList *next;

> -    char padding[];

> -} GenericList;

> -

>   /* This struct is layout-compatible with all Alternate types

>    * created by the QAPI generator. */

>   typedef struct GenericAlternate {

> diff --git a/include/qapi/util.h b/include/qapi/util.h

> index a7c3c6414874..50201896c7a4 100644

> --- a/include/qapi/util.h

> +++ b/include/qapi/util.h

> @@ -11,6 +11,14 @@

>   #ifndef QAPI_UTIL_H

>   #define QAPI_UTIL_H

> 

> +/* This struct is layout-compatible with all other *List structs

> + * created by the QAPI generator.  It is used as a typical

> + * singly-linked list. */


doesn't checkpatch complain for comment style?

> +typedef struct GenericList {

> +    struct GenericList *next;

> +    char padding[];

> +} GenericList;

> +

>   typedef struct QEnumLookup {

>       const char *const *array;

>       int size;

> 



-- 
Best regards,
Vladimir
Markus Armbruster Oct. 26, 2020, 2:18 p.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> Placing GenericList in util.h will make it easier for the next patch

> to promote QAPI_LIST_ADD() into a public macro without requiring more

> files to include the unrelated visitor.h.


Is this true?

You don't actually need GenericList to make use of QAPI_LIST_ADD(), do
you?  Any QAPI list type should do.

> However, we can't also move GenericAlternate; this is because it would

> introduce a circular dependency: qapi-builtin-types.h needs a complete

> definition of QEnumLookup (so it includes qapi/util.h), and

> GenericAlternate needs a complete definition of QType (declared in

> qapi-builtin-types.h).  Leaving GenericAlternate in visitor.h breaks

> the cycle, and doesn't matter since we don't have any further planned

> uses for that type outside of visitors.

>

> Suggested-by: Markus Armbruster <armbru@redhat.com>


I did suggest to consider moving GenericList and GenericAlternate next
to QAPI_LIST_ADD(), because they're (loosely) related.  We can't move
GenericAlternate.  Moving only GenericList brings GenericList and
QAPI_LIST_ADD() together, but separates the more closely related
GenericList and GenericAlternate.  Meh.

I'd leave it put.

> Signed-off-by: Eric Blake <eblake@redhat.com>

> ---

>  include/qapi/visitor.h | 9 +--------

>  include/qapi/util.h    | 8 ++++++++

>  2 files changed, 9 insertions(+), 8 deletions(-)

>

> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h

> index ebc19ede7fff..8c2e1c29ad8b 100644

> --- a/include/qapi/visitor.h

> +++ b/include/qapi/visitor.h

> @@ -16,6 +16,7 @@

>  #define QAPI_VISITOR_H

>

>  #include "qapi/qapi-builtin-types.h"

> +#include "qapi/util.h"


Not necessary, qapi-builtin-types.h must include it for QEnumLookup.

>  /*

>   * The QAPI schema defines both a set of C data types, and a QMP wire

> @@ -228,14 +229,6 @@

>

>  /*** Useful types ***/

>

> -/* This struct is layout-compatible with all other *List structs

> - * created by the QAPI generator.  It is used as a typical

> - * singly-linked list. */

> -typedef struct GenericList {

> -    struct GenericList *next;

> -    char padding[];

> -} GenericList;

> -

>  /* This struct is layout-compatible with all Alternate types

>   * created by the QAPI generator. */

>  typedef struct GenericAlternate {

> diff --git a/include/qapi/util.h b/include/qapi/util.h

> index a7c3c6414874..50201896c7a4 100644

> --- a/include/qapi/util.h

> +++ b/include/qapi/util.h

> @@ -11,6 +11,14 @@

>  #ifndef QAPI_UTIL_H

>  #define QAPI_UTIL_H

>

> +/* This struct is layout-compatible with all other *List structs

> + * created by the QAPI generator.  It is used as a typical

> + * singly-linked list. */

> +typedef struct GenericList {

> +    struct GenericList *next;

> +    char padding[];

> +} GenericList;

> +

>  typedef struct QEnumLookup {

>      const char *const *array;

>      int size;
Eric Blake Oct. 26, 2020, 2:22 p.m. UTC | #3
On 10/26/20 9:18 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Placing GenericList in util.h will make it easier for the next patch
>> to promote QAPI_LIST_ADD() into a public macro without requiring more
>> files to include the unrelated visitor.h.
> 
> Is this true?
> 
> You don't actually need GenericList to make use of QAPI_LIST_ADD(), do
> you?  Any QAPI list type should do.

Correct, compilation still works if I drop this patch.

> 
>> However, we can't also move GenericAlternate; this is because it would
>> introduce a circular dependency: qapi-builtin-types.h needs a complete
>> definition of QEnumLookup (so it includes qapi/util.h), and
>> GenericAlternate needs a complete definition of QType (declared in
>> qapi-builtin-types.h).  Leaving GenericAlternate in visitor.h breaks
>> the cycle, and doesn't matter since we don't have any further planned
>> uses for that type outside of visitors.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
> 
> I did suggest to consider moving GenericList and GenericAlternate next
> to QAPI_LIST_ADD(), because they're (loosely) related.  We can't move
> GenericAlternate.  Moving only GenericList brings GenericList and
> QAPI_LIST_ADD() together, but separates the more closely related
> GenericList and GenericAlternate.  Meh.
> 
> I'd leave it put.

Agreed. Dropping this patch in v6.
diff mbox series

Patch

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index ebc19ede7fff..8c2e1c29ad8b 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -16,6 +16,7 @@ 
 #define QAPI_VISITOR_H

 #include "qapi/qapi-builtin-types.h"
+#include "qapi/util.h"

 /*
  * The QAPI schema defines both a set of C data types, and a QMP wire
@@ -228,14 +229,6 @@ 

 /*** Useful types ***/

-/* This struct is layout-compatible with all other *List structs
- * created by the QAPI generator.  It is used as a typical
- * singly-linked list. */
-typedef struct GenericList {
-    struct GenericList *next;
-    char padding[];
-} GenericList;
-
 /* This struct is layout-compatible with all Alternate types
  * created by the QAPI generator. */
 typedef struct GenericAlternate {
diff --git a/include/qapi/util.h b/include/qapi/util.h
index a7c3c6414874..50201896c7a4 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,6 +11,14 @@ 
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H

+/* This struct is layout-compatible with all other *List structs
+ * created by the QAPI generator.  It is used as a typical
+ * singly-linked list. */
+typedef struct GenericList {
+    struct GenericList *next;
+    char padding[];
+} GenericList;
+
 typedef struct QEnumLookup {
     const char *const *array;
     int size;