* [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:23 ` Marc-André Lureau
2020-11-19 9:37 ` Markus Armbruster
2020-11-16 22:41 ` [PATCH v2 2/8] qnum: Make qnum_get_double() get const pointer Eduardo Habkost
` (7 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
Render existing doc comments at docs/devel/qobject.html.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
docs/devel/index.rst | 1 +
docs/devel/qobject.rst | 11 +++++++++
include/qapi/qmp/qnum.h | 4 +++-
include/qapi/qmp/qobject.h | 48 +++++++++++++++++++++++++-------------
qobject/qnum.c | 19 ++++++++++++---
5 files changed, 63 insertions(+), 20 deletions(-)
create mode 100644 docs/devel/qobject.rst
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index f10ed77e4c..1cb39a9384 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -35,3 +35,4 @@ Contents:
clocks
qom
block-coroutine-wrapper
+ qobject
diff --git a/docs/devel/qobject.rst b/docs/devel/qobject.rst
new file mode 100644
index 0000000000..4f192ced7c
--- /dev/null
+++ b/docs/devel/qobject.rst
@@ -0,0 +1,11 @@
+QObject API
+===========
+
+.. kernel-doc:: include/qapi/qmp/qobject.h
+
+QNum module
+-----------
+
+.. kernel-doc:: include/qapi/qmp/qnum.h
+
+.. kernel-doc:: qobject/qnum.c
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index bbae0a5ec8..25f4733efc 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -23,7 +23,9 @@ typedef enum {
QNUM_DOUBLE
} QNumKind;
-/*
+/**
+ * DOC:
+ *
* QNum encapsulates how our dialect of JSON fills in the blanks left
* by the JSON specification (RFC 8259) regarding numbers.
*
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index fcfd549220..bdc33bdb65 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -1,5 +1,5 @@
/*
- * QEMU Object Model.
+ * QObject API
*
* Based on ideas by Avi Kivity <avi@redhat.com>
*
@@ -10,24 +10,31 @@
*
* This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
* See the COPYING.LIB file in the top-level directory.
+ */
+
+/**
+ * DOC: QObject Reference Counts Terminology
*
- * QObject Reference Counts Terminology
- * ------------------------------------
+ * Returning references
+ * --------------------
*
- * - Returning references: A function that returns an object may
- * return it as either a weak or a strong reference. If the
- * reference is strong, you are responsible for calling
- * qobject_unref() on the reference when you are done.
+ * A function that returns an object may return it as either a
+ * weak or a strong reference. If the reference is strong, you
+ * are responsible for calling qobject_unref() on the reference
+ * when you are done.
*
- * If the reference is weak, the owner of the reference may free it at
- * any time in the future. Before storing the reference anywhere, you
- * should call qobject_ref() to make the reference strong.
+ * If the reference is weak, the owner of the reference may free it at
+ * any time in the future. Before storing the reference anywhere, you
+ * should call qobject_ref() to make the reference strong.
*
- * - Transferring ownership: when you transfer ownership of a reference
- * by calling a function, you are no longer responsible for calling
- * qobject_unref() when the reference is no longer needed. In other words,
- * when the function returns you must behave as if the reference to the
- * passed object was weak.
+ * Transferring ownership
+ * ----------------------
+ *
+ * When you transfer ownership of a reference by calling a
+ * function, you are no longer responsible for calling
+ * qobject_unref() when the reference is no longer needed. In
+ * other words, when the function returns you must behave as if
+ * the reference to the passed object was weak.
*/
#ifndef QOBJECT_H
#define QOBJECT_H
@@ -81,6 +88,8 @@ static inline void qobject_ref_impl(QObject *obj)
/**
* qobject_is_equal(): Return whether the two objects are equal.
+ * @x: QObject pointer
+ * @y: QObject pointer
*
* Any of the pointers may be NULL; return true if both are. Always
* return false if only one is (therefore a QNull object is not
@@ -90,6 +99,7 @@ bool qobject_is_equal(const QObject *x, const QObject *y);
/**
* qobject_destroy(): Free resources used by the object
+ * @obj: QObject pointer
*/
void qobject_destroy(QObject *obj);
@@ -103,6 +113,7 @@ static inline void qobject_unref_impl(QObject *obj)
/**
* qobject_ref(): Increment QObject's reference count
+ * @obj: QObject pointer
*
* Returns: the same @obj. The type of @obj will be propagated to the
* return type.
@@ -115,12 +126,14 @@ static inline void qobject_unref_impl(QObject *obj)
/**
* qobject_unref(): Decrement QObject's reference count, deallocate
- * when it reaches zero
+ * when it reaches zero
+ * @obj: QObject pointer
*/
#define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj))
/**
* qobject_type(): Return the QObject's type
+ * @obj: QObject pointer
*/
static inline QType qobject_type(const QObject *obj)
{
@@ -130,6 +143,9 @@ static inline QType qobject_type(const QObject *obj)
/**
* qobject_check_type(): Helper function for the qobject_to() macro.
+ * @obj: QObject pointer
+ * @type: Expected type of QObject
+ *
* Return @obj, but only if @obj is not NULL and @type is equal to
* @obj's type. Return NULL otherwise.
*/
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 7012fc57f2..017c8aa739 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -17,6 +17,7 @@
/**
* qnum_from_int(): Create a new QNum from an int64_t
+ * @value: int64_t value
*
* Return strong reference.
*/
@@ -33,6 +34,7 @@ QNum *qnum_from_int(int64_t value)
/**
* qnum_from_uint(): Create a new QNum from an uint64_t
+ * @value: uint64_t value
*
* Return strong reference.
*/
@@ -49,6 +51,7 @@ QNum *qnum_from_uint(uint64_t value)
/**
* qnum_from_double(): Create a new QNum from a double
+ * @value: double value
*
* Return strong reference.
*/
@@ -65,6 +68,8 @@ QNum *qnum_from_double(double value)
/**
* qnum_get_try_int(): Get an integer representation of the number
+ * @qn: QNum object
+ * @val: pointer to value
*
* Return true on success.
*/
@@ -90,6 +95,7 @@ bool qnum_get_try_int(const QNum *qn, int64_t *val)
/**
* qnum_get_int(): Get an integer representation of the number
+ * @qn: QNum object
*
* assert() on failure.
*/
@@ -102,7 +108,9 @@ int64_t qnum_get_int(const QNum *qn)
}
/**
- * qnum_get_uint(): Get an unsigned integer from the number
+ * qnum_value_get_try_uint(): Get an unsigned integer from the number
+ * @qn: QNum object
+ * @val: pointer to value
*
* Return true on success.
*/
@@ -128,6 +136,7 @@ bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
/**
* qnum_get_uint(): Get an unsigned integer from the number
+ * @qn: QNum object
*
* assert() on failure.
*/
@@ -141,6 +150,7 @@ uint64_t qnum_get_uint(const QNum *qn)
/**
* qnum_get_double(): Get a float representation of the number
+ * @qn: QNum object
*
* qnum_get_double() loses precision for integers beyond 53 bits.
*/
@@ -200,6 +210,8 @@ char *qnum_to_string(QNum *qn)
/**
* qnum_is_equal(): Test whether the two QNums are equal
+ * @x: QNum object
+ * @y: QNum object
*
* Negative integers are never considered equal to unsigned integers,
* but positive integers in the range [0, INT64_MAX] are considered
@@ -253,8 +265,9 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
}
/**
- * qnum_destroy_obj(): Free all memory allocated by a
- * QNum object
+ * qnum_destroy_obj(): Free all memory allocated by a QNum object
+ *
+ * @obj: QNum object to be destroyed
*/
void qnum_destroy_obj(QObject *obj)
{
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html
2020-11-16 22:41 ` [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html Eduardo Habkost
@ 2020-11-17 8:23 ` Marc-André Lureau
2020-11-19 9:37 ` Markus Armbruster
1 sibling, 0 replies; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:23 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 8523 bytes --]
On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> Render existing doc comments at docs/devel/qobject.html.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
> docs/devel/index.rst | 1 +
> docs/devel/qobject.rst | 11 +++++++++
> include/qapi/qmp/qnum.h | 4 +++-
> include/qapi/qmp/qobject.h | 48 +++++++++++++++++++++++++-------------
> qobject/qnum.c | 19 ++++++++++++---
> 5 files changed, 63 insertions(+), 20 deletions(-)
> create mode 100644 docs/devel/qobject.rst
>
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index f10ed77e4c..1cb39a9384 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -35,3 +35,4 @@ Contents:
> clocks
> qom
> block-coroutine-wrapper
> + qobject
> diff --git a/docs/devel/qobject.rst b/docs/devel/qobject.rst
> new file mode 100644
> index 0000000000..4f192ced7c
> --- /dev/null
> +++ b/docs/devel/qobject.rst
> @@ -0,0 +1,11 @@
> +QObject API
> +===========
> +
> +.. kernel-doc:: include/qapi/qmp/qobject.h
> +
> +QNum module
> +-----------
> +
> +.. kernel-doc:: include/qapi/qmp/qnum.h
> +
> +.. kernel-doc:: qobject/qnum.c
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index bbae0a5ec8..25f4733efc 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -23,7 +23,9 @@ typedef enum {
> QNUM_DOUBLE
> } QNumKind;
>
> -/*
> +/**
> + * DOC:
> + *
> * QNum encapsulates how our dialect of JSON fills in the blanks left
> * by the JSON specification (RFC 8259) regarding numbers.
> *
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index fcfd549220..bdc33bdb65 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -1,5 +1,5 @@
> /*
> - * QEMU Object Model.
> + * QObject API
> *
> * Based on ideas by Avi Kivity <avi@redhat.com>
> *
> @@ -10,24 +10,31 @@
> *
> * This work is licensed under the terms of the GNU LGPL, version 2.1 or
> later.
> * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +/**
> + * DOC: QObject Reference Counts Terminology
> *
> - * QObject Reference Counts Terminology
> - * ------------------------------------
> + * Returning references
> + * --------------------
> *
> - * - Returning references: A function that returns an object may
> - * return it as either a weak or a strong reference. If the
> - * reference is strong, you are responsible for calling
> - * qobject_unref() on the reference when you are done.
> + * A function that returns an object may return it as either a
> + * weak or a strong reference. If the reference is strong, you
> + * are responsible for calling qobject_unref() on the reference
> + * when you are done.
> *
> - * If the reference is weak, the owner of the reference may free it at
> - * any time in the future. Before storing the reference anywhere, you
> - * should call qobject_ref() to make the reference strong.
> + * If the reference is weak, the owner of the reference may free it at
> + * any time in the future. Before storing the reference anywhere, you
> + * should call qobject_ref() to make the reference strong.
> *
> - * - Transferring ownership: when you transfer ownership of a reference
> - * by calling a function, you are no longer responsible for calling
> - * qobject_unref() when the reference is no longer needed. In other
> words,
> - * when the function returns you must behave as if the reference to the
> - * passed object was weak.
> + * Transferring ownership
> + * ----------------------
> + *
> + * When you transfer ownership of a reference by calling a
> + * function, you are no longer responsible for calling
> + * qobject_unref() when the reference is no longer needed. In
> + * other words, when the function returns you must behave as if
> + * the reference to the passed object was weak.
> */
> #ifndef QOBJECT_H
> #define QOBJECT_H
> @@ -81,6 +88,8 @@ static inline void qobject_ref_impl(QObject *obj)
>
> /**
> * qobject_is_equal(): Return whether the two objects are equal.
> + * @x: QObject pointer
> + * @y: QObject pointer
> *
> * Any of the pointers may be NULL; return true if both are. Always
> * return false if only one is (therefore a QNull object is not
> @@ -90,6 +99,7 @@ bool qobject_is_equal(const QObject *x, const QObject
> *y);
>
> /**
> * qobject_destroy(): Free resources used by the object
> + * @obj: QObject pointer
> */
> void qobject_destroy(QObject *obj);
>
> @@ -103,6 +113,7 @@ static inline void qobject_unref_impl(QObject *obj)
>
> /**
> * qobject_ref(): Increment QObject's reference count
> + * @obj: QObject pointer
> *
> * Returns: the same @obj. The type of @obj will be propagated to the
> * return type.
> @@ -115,12 +126,14 @@ static inline void qobject_unref_impl(QObject *obj)
>
> /**
> * qobject_unref(): Decrement QObject's reference count, deallocate
> - * when it reaches zero
> + * when it reaches zero
> + * @obj: QObject pointer
> */
> #define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj))
>
> /**
> * qobject_type(): Return the QObject's type
> + * @obj: QObject pointer
> */
> static inline QType qobject_type(const QObject *obj)
> {
> @@ -130,6 +143,9 @@ static inline QType qobject_type(const QObject *obj)
>
> /**
> * qobject_check_type(): Helper function for the qobject_to() macro.
> + * @obj: QObject pointer
> + * @type: Expected type of QObject
> + *
> * Return @obj, but only if @obj is not NULL and @type is equal to
> * @obj's type. Return NULL otherwise.
> */
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 7012fc57f2..017c8aa739 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -17,6 +17,7 @@
>
> /**
> * qnum_from_int(): Create a new QNum from an int64_t
> + * @value: int64_t value
> *
> * Return strong reference.
> */
> @@ -33,6 +34,7 @@ QNum *qnum_from_int(int64_t value)
>
> /**
> * qnum_from_uint(): Create a new QNum from an uint64_t
> + * @value: uint64_t value
> *
> * Return strong reference.
> */
> @@ -49,6 +51,7 @@ QNum *qnum_from_uint(uint64_t value)
>
> /**
> * qnum_from_double(): Create a new QNum from a double
> + * @value: double value
> *
> * Return strong reference.
> */
> @@ -65,6 +68,8 @@ QNum *qnum_from_double(double value)
>
> /**
> * qnum_get_try_int(): Get an integer representation of the number
> + * @qn: QNum object
> + * @val: pointer to value
> *
> * Return true on success.
> */
> @@ -90,6 +95,7 @@ bool qnum_get_try_int(const QNum *qn, int64_t *val)
>
> /**
> * qnum_get_int(): Get an integer representation of the number
> + * @qn: QNum object
> *
> * assert() on failure.
> */
> @@ -102,7 +108,9 @@ int64_t qnum_get_int(const QNum *qn)
> }
>
> /**
> - * qnum_get_uint(): Get an unsigned integer from the number
> + * qnum_value_get_try_uint(): Get an unsigned integer from the number
> + * @qn: QNum object
> + * @val: pointer to value
> *
> * Return true on success.
> */
> @@ -128,6 +136,7 @@ bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
>
> /**
> * qnum_get_uint(): Get an unsigned integer from the number
> + * @qn: QNum object
> *
> * assert() on failure.
> */
> @@ -141,6 +150,7 @@ uint64_t qnum_get_uint(const QNum *qn)
>
> /**
> * qnum_get_double(): Get a float representation of the number
> + * @qn: QNum object
> *
> * qnum_get_double() loses precision for integers beyond 53 bits.
> */
> @@ -200,6 +210,8 @@ char *qnum_to_string(QNum *qn)
>
> /**
> * qnum_is_equal(): Test whether the two QNums are equal
> + * @x: QNum object
> + * @y: QNum object
> *
> * Negative integers are never considered equal to unsigned integers,
> * but positive integers in the range [0, INT64_MAX] are considered
> @@ -253,8 +265,9 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> }
>
> /**
> - * qnum_destroy_obj(): Free all memory allocated by a
> - * QNum object
> + * qnum_destroy_obj(): Free all memory allocated by a QNum object
> + *
> + * @obj: QNum object to be destroyed
> */
> void qnum_destroy_obj(QObject *obj)
> {
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 9949 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html
2020-11-16 22:41 ` [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html Eduardo Habkost
2020-11-17 8:23 ` Marc-André Lureau
@ 2020-11-19 9:37 ` Markus Armbruster
2020-11-19 18:03 ` Eduardo Habkost
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-19 9:37 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> Render existing doc comments at docs/devel/qobject.html.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> docs/devel/index.rst | 1 +
> docs/devel/qobject.rst | 11 +++++++++
> include/qapi/qmp/qnum.h | 4 +++-
> include/qapi/qmp/qobject.h | 48 +++++++++++++++++++++++++-------------
> qobject/qnum.c | 19 ++++++++++++---
> 5 files changed, 63 insertions(+), 20 deletions(-)
> create mode 100644 docs/devel/qobject.rst
>
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index f10ed77e4c..1cb39a9384 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -35,3 +35,4 @@ Contents:
> clocks
> qom
> block-coroutine-wrapper
> + qobject
> diff --git a/docs/devel/qobject.rst b/docs/devel/qobject.rst
> new file mode 100644
> index 0000000000..4f192ced7c
> --- /dev/null
> +++ b/docs/devel/qobject.rst
> @@ -0,0 +1,11 @@
> +QObject API
> +===========
> +
> +.. kernel-doc:: include/qapi/qmp/qobject.h
> +
> +QNum module
> +-----------
> +
> +.. kernel-doc:: include/qapi/qmp/qnum.h
> +
> +.. kernel-doc:: qobject/qnum.c
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index bbae0a5ec8..25f4733efc 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -23,7 +23,9 @@ typedef enum {
> QNUM_DOUBLE
> } QNumKind;
>
> -/*
> +/**
> + * DOC:
> + *
> * QNum encapsulates how our dialect of JSON fills in the blanks left
> * by the JSON specification (RFC 8259) regarding numbers.
> *
Educate me: what does DOC: do?
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index fcfd549220..bdc33bdb65 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -1,5 +1,5 @@
> /*
> - * QEMU Object Model.
> + * QObject API
> *
> * Based on ideas by Avi Kivity <avi@redhat.com>
> *
> @@ -10,24 +10,31 @@
> *
> * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +/**
> + * DOC: QObject Reference Counts Terminology
> *
> - * QObject Reference Counts Terminology
> - * ------------------------------------
> + * Returning references
> + * --------------------
> *
> - * - Returning references: A function that returns an object may
> - * return it as either a weak or a strong reference. If the
> - * reference is strong, you are responsible for calling
> - * qobject_unref() on the reference when you are done.
> + * A function that returns an object may return it as either a
> + * weak or a strong reference. If the reference is strong, you
> + * are responsible for calling qobject_unref() on the reference
> + * when you are done.
> *
> - * If the reference is weak, the owner of the reference may free it at
> - * any time in the future. Before storing the reference anywhere, you
> - * should call qobject_ref() to make the reference strong.
> + * If the reference is weak, the owner of the reference may free it at
> + * any time in the future. Before storing the reference anywhere, you
> + * should call qobject_ref() to make the reference strong.
> *
> - * - Transferring ownership: when you transfer ownership of a reference
> - * by calling a function, you are no longer responsible for calling
> - * qobject_unref() when the reference is no longer needed. In other words,
> - * when the function returns you must behave as if the reference to the
> - * passed object was weak.
> + * Transferring ownership
> + * ----------------------
> + *
> + * When you transfer ownership of a reference by calling a
> + * function, you are no longer responsible for calling
> + * qobject_unref() when the reference is no longer needed. In
> + * other words, when the function returns you must behave as if
> + * the reference to the passed object was weak.
> */
> #ifndef QOBJECT_H
> #define QOBJECT_H
> @@ -81,6 +88,8 @@ static inline void qobject_ref_impl(QObject *obj)
>
> /**
> * qobject_is_equal(): Return whether the two objects are equal.
> + * @x: QObject pointer
> + * @y: QObject pointer
> *
> * Any of the pointers may be NULL; return true if both are. Always
> * return false if only one is (therefore a QNull object is not
> @@ -90,6 +99,7 @@ bool qobject_is_equal(const QObject *x, const QObject *y);
>
> /**
> * qobject_destroy(): Free resources used by the object
> + * @obj: QObject pointer
> */
> void qobject_destroy(QObject *obj);
>
> @@ -103,6 +113,7 @@ static inline void qobject_unref_impl(QObject *obj)
>
> /**
> * qobject_ref(): Increment QObject's reference count
> + * @obj: QObject pointer
> *
> * Returns: the same @obj. The type of @obj will be propagated to the
> * return type.
> @@ -115,12 +126,14 @@ static inline void qobject_unref_impl(QObject *obj)
>
> /**
> * qobject_unref(): Decrement QObject's reference count, deallocate
> - * when it reaches zero
> + * when it reaches zero
> + * @obj: QObject pointer
> */
> #define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj))
>
> /**
> * qobject_type(): Return the QObject's type
> + * @obj: QObject pointer
> */
> static inline QType qobject_type(const QObject *obj)
> {
> @@ -130,6 +143,9 @@ static inline QType qobject_type(const QObject *obj)
>
> /**
> * qobject_check_type(): Helper function for the qobject_to() macro.
> + * @obj: QObject pointer
> + * @type: Expected type of QObject
> + *
> * Return @obj, but only if @obj is not NULL and @type is equal to
> * @obj's type. Return NULL otherwise.
> */
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 7012fc57f2..017c8aa739 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -17,6 +17,7 @@
>
> /**
> * qnum_from_int(): Create a new QNum from an int64_t
> + * @value: int64_t value
> *
> * Return strong reference.
> */
> @@ -33,6 +34,7 @@ QNum *qnum_from_int(int64_t value)
>
> /**
> * qnum_from_uint(): Create a new QNum from an uint64_t
> + * @value: uint64_t value
> *
> * Return strong reference.
> */
> @@ -49,6 +51,7 @@ QNum *qnum_from_uint(uint64_t value)
>
> /**
> * qnum_from_double(): Create a new QNum from a double
> + * @value: double value
> *
> * Return strong reference.
> */
> @@ -65,6 +68,8 @@ QNum *qnum_from_double(double value)
>
> /**
> * qnum_get_try_int(): Get an integer representation of the number
> + * @qn: QNum object
> + * @val: pointer to value
> *
> * Return true on success.
> */
> @@ -90,6 +95,7 @@ bool qnum_get_try_int(const QNum *qn, int64_t *val)
>
> /**
> * qnum_get_int(): Get an integer representation of the number
> + * @qn: QNum object
> *
> * assert() on failure.
> */
> @@ -102,7 +108,9 @@ int64_t qnum_get_int(const QNum *qn)
> }
>
> /**
> - * qnum_get_uint(): Get an unsigned integer from the number
> + * qnum_value_get_try_uint(): Get an unsigned integer from the number
> + * @qn: QNum object
> + * @val: pointer to value
> *
> * Return true on success.
> */
> @@ -128,6 +136,7 @@ bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
>
> /**
> * qnum_get_uint(): Get an unsigned integer from the number
> + * @qn: QNum object
> *
> * assert() on failure.
> */
> @@ -141,6 +150,7 @@ uint64_t qnum_get_uint(const QNum *qn)
>
> /**
> * qnum_get_double(): Get a float representation of the number
> + * @qn: QNum object
> *
> * qnum_get_double() loses precision for integers beyond 53 bits.
> */
> @@ -200,6 +210,8 @@ char *qnum_to_string(QNum *qn)
>
> /**
> * qnum_is_equal(): Test whether the two QNums are equal
> + * @x: QNum object
> + * @y: QNum object
> *
> * Negative integers are never considered equal to unsigned integers,
> * but positive integers in the range [0, INT64_MAX] are considered
> @@ -253,8 +265,9 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> }
>
> /**
> - * qnum_destroy_obj(): Free all memory allocated by a
> - * QNum object
> + * qnum_destroy_obj(): Free all memory allocated by a QNum object
> + *
> + * @obj: QNum object to be destroyed
> */
> void qnum_destroy_obj(QObject *obj)
> {
Many lines of the form
+ * @foo: a foo
One of my reasons to dislike GTK-Doc. Oh well, it's what we're using.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html
2020-11-19 9:37 ` Markus Armbruster
@ 2020-11-19 18:03 ` Eduardo Habkost
0 siblings, 0 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 18:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel, Peter Maydell
CCing Peter, Sphinx documentation machinery maintainer.
On Thu, Nov 19, 2020 at 10:37:22AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > Render existing doc comments at docs/devel/qobject.html.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
[...]
> > /**
> > - * qnum_destroy_obj(): Free all memory allocated by a
> > - * QNum object
> > + * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > + *
> > + * @obj: QNum object to be destroyed
> > */
> > void qnum_destroy_obj(QObject *obj)
> > {
>
> Many lines of the form
>
> + * @foo: a foo
I hate them too.
>
> One of my reasons to dislike GTK-Doc. Oh well, it's what we're using.
It's not gtk-doc. It's kernel-doc who generates warnings
if some parameters are not documented, and maybe we should
silence them.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 2/8] qnum: Make qnum_get_double() get const pointer
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
2020-11-16 22:41 ` [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:26 ` Marc-André Lureau
2020-11-16 22:41 ` [PATCH v2 3/8] qnum: QNumValue type for QNum value literals Eduardo Habkost
` (6 subsequent siblings)
8 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
qnum_get_double() won't change the object, the argument can be
const.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/qapi/qmp/qnum.h | 2 +-
qobject/qnum.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 25f4733efc..55c27b1c24 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -66,7 +66,7 @@ int64_t qnum_get_int(const QNum *qn);
bool qnum_get_try_uint(const QNum *qn, uint64_t *val);
uint64_t qnum_get_uint(const QNum *qn);
-double qnum_get_double(QNum *qn);
+double qnum_get_double(const QNum *qn);
char *qnum_to_string(QNum *qn);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 017c8aa739..69fd9a82d9 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -154,7 +154,7 @@ uint64_t qnum_get_uint(const QNum *qn)
*
* qnum_get_double() loses precision for integers beyond 53 bits.
*/
-double qnum_get_double(QNum *qn)
+double qnum_get_double(const QNum *qn)
{
switch (qn->kind) {
case QNUM_I64:
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 2/8] qnum: Make qnum_get_double() get const pointer
2020-11-16 22:41 ` [PATCH v2 2/8] qnum: Make qnum_get_double() get const pointer Eduardo Habkost
@ 2020-11-17 8:26 ` Marc-André Lureau
0 siblings, 0 replies; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:26 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]
On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> qnum_get_double() won't change the object, the argument can be
> const.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
No idea why I didn't make it const in the first place.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
> include/qapi/qmp/qnum.h | 2 +-
> qobject/qnum.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index 25f4733efc..55c27b1c24 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -66,7 +66,7 @@ int64_t qnum_get_int(const QNum *qn);
> bool qnum_get_try_uint(const QNum *qn, uint64_t *val);
> uint64_t qnum_get_uint(const QNum *qn);
>
> -double qnum_get_double(QNum *qn);
> +double qnum_get_double(const QNum *qn);
>
> char *qnum_to_string(QNum *qn);
>
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 017c8aa739..69fd9a82d9 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -154,7 +154,7 @@ uint64_t qnum_get_uint(const QNum *qn)
> *
> * qnum_get_double() loses precision for integers beyond 53 bits.
> */
> -double qnum_get_double(QNum *qn)
> +double qnum_get_double(const QNum *qn)
> {
> switch (qn->kind) {
> case QNUM_I64:
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 2205 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
2020-11-16 22:41 ` [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html Eduardo Habkost
2020-11-16 22:41 ` [PATCH v2 2/8] qnum: Make qnum_get_double() get const pointer Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:37 ` Marc-André Lureau
2020-11-16 22:41 ` [PATCH v2 4/8] qnum: qnum_value_is_equal() function Eduardo Habkost
` (5 subsequent siblings)
8 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
Provide a separate QNumValue type that can be used for QNum value
literals without the referencing counting and memory allocation
features provided by QObject.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Fix "make check" failure, by updating check-qnum unit test to
use the new struct fields
---
include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
qobject/qnum.c | 78 ++++++++++++++++++++---------------------
tests/check-qnum.c | 14 ++++----
3 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 55c27b1c24..62fbdfda68 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -46,20 +46,56 @@ typedef enum {
* in range: qnum_get_try_int() / qnum_get_try_uint() check range and
* convert under the hood.
*/
-struct QNum {
- struct QObjectBase_ base;
+
+/**
+ * struct QNumValue: the value of a QNum
+ *
+ * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
+ * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
+ */
+typedef struct QNumValue {
+ /* private: */
QNumKind kind;
union {
int64_t i64;
uint64_t u64;
double dbl;
} u;
+} QNumValue;
+
+#define QNUM_VAL_INT(value) \
+ { .kind = QNUM_I64, .u.i64 = value }
+#define QNUM_VAL_UINT(value) \
+ { .kind = QNUM_U64, .u.u64 = value }
+#define QNUM_VAL_DOUBLE(value) \
+ { .kind = QNUM_DOUBLE, .u.dbl = value }
+
+struct QNum {
+ struct QObjectBase_ base;
+ QNumValue value;
};
+/**
+ * qnum_from_int(): Create a new QNum from a QNumValue
+ * @value: QNumValue
+ *
+ * Return strong reference.
+ */
+QNum *qnum_from_value(QNumValue value);
+
QNum *qnum_from_int(int64_t value);
QNum *qnum_from_uint(uint64_t value);
QNum *qnum_from_double(double value);
+/**
+ * qnum_get_value(): Get QNumValue from QNum
+ * @qn: QNum object
+ */
+static inline const QNumValue *qnum_get_value(const QNum *qn)
+{
+ return &qn->value;
+}
+
bool qnum_get_try_int(const QNum *qn, int64_t *val);
int64_t qnum_get_int(const QNum *qn);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 69fd9a82d9..f80d4efd76 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -15,6 +15,15 @@
#include "qemu/osdep.h"
#include "qapi/qmp/qnum.h"
+QNum *qnum_from_value(QNumValue value)
+{
+ QNum *qn = g_new(QNum, 1);
+
+ qobject_init(QOBJECT(qn), QTYPE_QNUM);
+ qn->value = value;
+ return qn;
+}
+
/**
* qnum_from_int(): Create a new QNum from an int64_t
* @value: int64_t value
@@ -23,13 +32,7 @@
*/
QNum *qnum_from_int(int64_t value)
{
- QNum *qn = g_new(QNum, 1);
-
- qobject_init(QOBJECT(qn), QTYPE_QNUM);
- qn->kind = QNUM_I64;
- qn->u.i64 = value;
-
- return qn;
+ return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
}
/**
@@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
*/
QNum *qnum_from_uint(uint64_t value)
{
- QNum *qn = g_new(QNum, 1);
-
- qobject_init(QOBJECT(qn), QTYPE_QNUM);
- qn->kind = QNUM_U64;
- qn->u.u64 = value;
-
- return qn;
+ return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
}
/**
@@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
*/
QNum *qnum_from_double(double value)
{
- QNum *qn = g_new(QNum, 1);
-
- qobject_init(QOBJECT(qn), QTYPE_QNUM);
- qn->kind = QNUM_DOUBLE;
- qn->u.dbl = value;
-
- return qn;
+ return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
}
/**
@@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
*/
bool qnum_get_try_int(const QNum *qn, int64_t *val)
{
- switch (qn->kind) {
+ const QNumValue *qv = &qn->value;
+
+ switch (qv->kind) {
case QNUM_I64:
- *val = qn->u.i64;
+ *val = qv->u.i64;
return true;
case QNUM_U64:
- if (qn->u.u64 > INT64_MAX) {
+ if (qv->u.u64 > INT64_MAX) {
return false;
}
- *val = qn->u.u64;
+ *val = qv->u.u64;
return true;
case QNUM_DOUBLE:
return false;
@@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
*/
bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
{
- switch (qn->kind) {
+ const QNumValue *qv = &qn->value;
+
+ switch (qv->kind) {
case QNUM_I64:
- if (qn->u.i64 < 0) {
+ if (qv->u.i64 < 0) {
return false;
}
- *val = qn->u.i64;
+ *val = qv->u.i64;
return true;
case QNUM_U64:
- *val = qn->u.u64;
+ *val = qv->u.u64;
return true;
case QNUM_DOUBLE:
return false;
@@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
*/
double qnum_get_double(const QNum *qn)
{
- switch (qn->kind) {
+ const QNumValue *qv = &qn->value;
+
+ switch (qv->kind) {
case QNUM_I64:
- return qn->u.i64;
+ return qv->u.i64;
case QNUM_U64:
- return qn->u.u64;
+ return qv->u.u64;
case QNUM_DOUBLE:
- return qn->u.dbl;
+ return qv->u.dbl;
}
assert(0);
@@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
char *qnum_to_string(QNum *qn)
{
+ const QNumValue *qv = &qn->value;
char *buffer;
int len;
- switch (qn->kind) {
+ switch (qv->kind) {
case QNUM_I64:
- return g_strdup_printf("%" PRId64, qn->u.i64);
+ return g_strdup_printf("%" PRId64, qv->u.i64);
case QNUM_U64:
- return g_strdup_printf("%" PRIu64, qn->u.u64);
+ return g_strdup_printf("%" PRIu64, qv->u.u64);
case QNUM_DOUBLE:
/* FIXME: snprintf() is locale dependent; but JSON requires
* numbers to be formatted as if in the C locale. Dependence
@@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
* rounding errors; we should be using DBL_DECIMAL_DIG (17),
* and only rounding to a shorter number if the result would
* still produce the same floating point value. */
- buffer = g_strdup_printf("%f" , qn->u.dbl);
+ buffer = g_strdup_printf("%f" , qv->u.dbl);
len = strlen(buffer);
while (len > 0 && buffer[len - 1] == '0') {
len--;
@@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
*/
bool qnum_is_equal(const QObject *x, const QObject *y)
{
- QNum *num_x = qobject_to(QNum, x);
- QNum *num_y = qobject_to(QNum, y);
+ const QNum *qnum_x = qobject_to(QNum, x);
+ const QNum *qnum_y = qobject_to(QNum, y);
+ const QNumValue *num_x = &qnum_x->value;
+ const QNumValue *num_y = &qnum_y->value;
switch (num_x->kind) {
case QNUM_I64:
diff --git a/tests/check-qnum.c b/tests/check-qnum.c
index 4105015872..9499b0d845 100644
--- a/tests/check-qnum.c
+++ b/tests/check-qnum.c
@@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
qn = qnum_from_int(value);
g_assert(qn != NULL);
- g_assert_cmpint(qn->kind, ==, QNUM_I64);
- g_assert_cmpint(qn->u.i64, ==, value);
+ g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
+ g_assert_cmpint(qn->value.u.i64, ==, value);
g_assert_cmpint(qn->base.refcnt, ==, 1);
g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
@@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
qn = qnum_from_uint(value);
g_assert(qn != NULL);
- g_assert_cmpint(qn->kind, ==, QNUM_U64);
- g_assert(qn->u.u64 == value);
+ g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
+ g_assert(qn->value.u.u64 == value);
g_assert(qn->base.refcnt == 1);
g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
@@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
qn = qnum_from_double(value);
g_assert(qn != NULL);
- g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
- g_assert_cmpfloat(qn->u.dbl, ==, value);
+ g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
+ g_assert_cmpfloat(qn->value.u.dbl, ==, value);
g_assert_cmpint(qn->base.refcnt, ==, 1);
g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
@@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
const int64_t value = 0x1234567890abcdefLL;
qn = qnum_from_int(value);
- g_assert_cmpint((int64_t) qn->u.i64, ==, value);
+ g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
qobject_unref(qn);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-16 22:41 ` [PATCH v2 3/8] qnum: QNumValue type for QNum value literals Eduardo Habkost
@ 2020-11-17 8:37 ` Marc-André Lureau
2020-11-17 14:42 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:37 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 9560 bytes --]
On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> Provide a separate QNumValue type that can be used for QNum value
> literals without the referencing counting and memory allocation
> features provided by QObject.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Fix "make check" failure, by updating check-qnum unit test to
> use the new struct fields
> ---
> include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> qobject/qnum.c | 78 ++++++++++++++++++++---------------------
> tests/check-qnum.c | 14 ++++----
> 3 files changed, 84 insertions(+), 48 deletions(-)
>
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index 55c27b1c24..62fbdfda68 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -46,20 +46,56 @@ typedef enum {
> * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> * convert under the hood.
> */
> -struct QNum {
> - struct QObjectBase_ base;
> +
> +/**
> + * struct QNumValue: the value of a QNum
> + *
> + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> + */
> +typedef struct QNumValue {
> + /* private: */
> QNumKind kind;
> union {
> int64_t i64;
> uint64_t u64;
> double dbl;
> } u;
> +} QNumValue;
> +
> +#define QNUM_VAL_INT(value) \
> + { .kind = QNUM_I64, .u.i64 = value }
> +#define QNUM_VAL_UINT(value) \
> + { .kind = QNUM_U64, .u.u64 = value }
> +#define QNUM_VAL_DOUBLE(value) \
> + { .kind = QNUM_DOUBLE, .u.dbl = value }
> +
> +struct QNum {
> + struct QObjectBase_ base;
> + QNumValue value;
> };
>
> +/**
> + * qnum_from_int(): Create a new QNum from a QNumValue
>
Copy-pasta qnum_from_int() -> qnum_from_value()
I wonder if there is a check for that kind of mistake, as it may be common.
+ * @value: QNumValue
> + *
> + * Return strong reference.
> + */
> +QNum *qnum_from_value(QNumValue value);
>
+
> QNum *qnum_from_int(int64_t value);
> QNum *qnum_from_uint(uint64_t value);
> QNum *qnum_from_double(double value);
>
> +/**
> + * qnum_get_value(): Get QNumValue from QNum
> + * @qn: QNum object
> + */
> +static inline const QNumValue *qnum_get_value(const QNum *qn)
> +{
> + return &qn->value;
> +}
> +
>
Nothing uses that function in this patch. Should be put into use.
bool qnum_get_try_int(const QNum *qn, int64_t *val);
> int64_t qnum_get_int(const QNum *qn);
>
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 69fd9a82d9..f80d4efd76 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -15,6 +15,15 @@
> #include "qemu/osdep.h"
> #include "qapi/qmp/qnum.h"
>
> +QNum *qnum_from_value(QNumValue value)
>
I wonder if it shouldn't be made "inline" in header too (if that can help
avoid argument copy).
+{
> + QNum *qn = g_new(QNum, 1);
> +
> + qobject_init(QOBJECT(qn), QTYPE_QNUM);
> + qn->value = value;
> + return qn;
> +}
> +
> /**
> * qnum_from_int(): Create a new QNum from an int64_t
> * @value: int64_t value
> @@ -23,13 +32,7 @@
> */
> QNum *qnum_from_int(int64_t value)
> {
> - QNum *qn = g_new(QNum, 1);
> -
> - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> - qn->kind = QNUM_I64;
> - qn->u.i64 = value;
> -
> - return qn;
> + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> }
>
> /**
> @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> */
> QNum *qnum_from_uint(uint64_t value)
> {
> - QNum *qn = g_new(QNum, 1);
> -
> - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> - qn->kind = QNUM_U64;
> - qn->u.u64 = value;
> -
> - return qn;
> + return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> }
>
> /**
> @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> */
> QNum *qnum_from_double(double value)
> {
> - QNum *qn = g_new(QNum, 1);
> -
> - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> - qn->kind = QNUM_DOUBLE;
> - qn->u.dbl = value;
> -
> - return qn;
> + return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> }
>
> /**
> @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> */
> bool qnum_get_try_int(const QNum *qn, int64_t *val)
> {
> - switch (qn->kind) {
> + const QNumValue *qv = &qn->value;
> +
> + switch (qv->kind) {
> case QNUM_I64:
> - *val = qn->u.i64;
> + *val = qv->u.i64;
> return true;
> case QNUM_U64:
> - if (qn->u.u64 > INT64_MAX) {
> + if (qv->u.u64 > INT64_MAX) {
> return false;
> }
> - *val = qn->u.u64;
> + *val = qv->u.u64;
> return true;
> case QNUM_DOUBLE:
> return false;
> @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> */
> bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> {
> - switch (qn->kind) {
> + const QNumValue *qv = &qn->value;
> +
> + switch (qv->kind) {
> case QNUM_I64:
> - if (qn->u.i64 < 0) {
> + if (qv->u.i64 < 0) {
> return false;
> }
> - *val = qn->u.i64;
> + *val = qv->u.i64;
> return true;
> case QNUM_U64:
> - *val = qn->u.u64;
> + *val = qv->u.u64;
> return true;
> case QNUM_DOUBLE:
> return false;
> @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> */
> double qnum_get_double(const QNum *qn)
> {
> - switch (qn->kind) {
> + const QNumValue *qv = &qn->value;
> +
> + switch (qv->kind) {
> case QNUM_I64:
> - return qn->u.i64;
> + return qv->u.i64;
> case QNUM_U64:
> - return qn->u.u64;
> + return qv->u.u64;
> case QNUM_DOUBLE:
> - return qn->u.dbl;
> + return qv->u.dbl;
> }
>
> assert(0);
> @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
>
> char *qnum_to_string(QNum *qn)
> {
> + const QNumValue *qv = &qn->value;
>
qnum_get_value() ?
char *buffer;
> int len;
>
> - switch (qn->kind) {
> + switch (qv->kind) {
> case QNUM_I64:
> - return g_strdup_printf("%" PRId64, qn->u.i64);
> + return g_strdup_printf("%" PRId64, qv->u.i64);
> case QNUM_U64:
> - return g_strdup_printf("%" PRIu64, qn->u.u64);
> + return g_strdup_printf("%" PRIu64, qv->u.u64);
> case QNUM_DOUBLE:
> /* FIXME: snprintf() is locale dependent; but JSON requires
> * numbers to be formatted as if in the C locale. Dependence
> @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> * and only rounding to a shorter number if the result would
> * still produce the same floating point value. */
> - buffer = g_strdup_printf("%f" , qn->u.dbl);
> + buffer = g_strdup_printf("%f" , qv->u.dbl);
> len = strlen(buffer);
> while (len > 0 && buffer[len - 1] == '0') {
> len--;
> @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> */
> bool qnum_is_equal(const QObject *x, const QObject *y)
> {
> - QNum *num_x = qobject_to(QNum, x);
> - QNum *num_y = qobject_to(QNum, y);
> + const QNum *qnum_x = qobject_to(QNum, x);
> + const QNum *qnum_y = qobject_to(QNum, y);
> + const QNumValue *num_x = &qnum_x->value;
> + const QNumValue *num_y = &qnum_y->value;
>
> switch (num_x->kind) {
> case QNUM_I64:
> diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> index 4105015872..9499b0d845 100644
> --- a/tests/check-qnum.c
> +++ b/tests/check-qnum.c
> @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
>
> qn = qnum_from_int(value);
> g_assert(qn != NULL);
> - g_assert_cmpint(qn->kind, ==, QNUM_I64);
> - g_assert_cmpint(qn->u.i64, ==, value);
> + g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> + g_assert_cmpint(qn->value.u.i64, ==, value);
> g_assert_cmpint(qn->base.refcnt, ==, 1);
> g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>
> @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
>
> qn = qnum_from_uint(value);
> g_assert(qn != NULL);
> - g_assert_cmpint(qn->kind, ==, QNUM_U64);
> - g_assert(qn->u.u64 == value);
> + g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> + g_assert(qn->value.u.u64 == value);
> g_assert(qn->base.refcnt == 1);
> g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
>
> @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
>
> qn = qnum_from_double(value);
> g_assert(qn != NULL);
> - g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> - g_assert_cmpfloat(qn->u.dbl, ==, value);
> + g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> + g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> g_assert_cmpint(qn->base.refcnt, ==, 1);
> g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>
> @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> const int64_t value = 0x1234567890abcdefLL;
>
> qn = qnum_from_int(value);
> - g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> + g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
>
> qobject_unref(qn);
> }
> --
> 2.28.0
>
>
>
lgtm otherwise
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 12343 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-17 8:37 ` Marc-André Lureau
@ 2020-11-17 14:42 ` Eduardo Habkost
2020-11-17 14:58 ` Marc-André Lureau
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-17 14:42 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > Provide a separate QNumValue type that can be used for QNum value
> > literals without the referencing counting and memory allocation
> > features provided by QObject.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Fix "make check" failure, by updating check-qnum unit test to
> > use the new struct fields
> > ---
> > include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> > qobject/qnum.c | 78 ++++++++++++++++++++---------------------
> > tests/check-qnum.c | 14 ++++----
> > 3 files changed, 84 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > index 55c27b1c24..62fbdfda68 100644
> > --- a/include/qapi/qmp/qnum.h
> > +++ b/include/qapi/qmp/qnum.h
> > @@ -46,20 +46,56 @@ typedef enum {
> > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> > * convert under the hood.
> > */
> > -struct QNum {
> > - struct QObjectBase_ base;
> > +
> > +/**
> > + * struct QNumValue: the value of a QNum
> > + *
> > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> > + */
> > +typedef struct QNumValue {
> > + /* private: */
> > QNumKind kind;
> > union {
> > int64_t i64;
> > uint64_t u64;
> > double dbl;
> > } u;
> > +} QNumValue;
> > +
> > +#define QNUM_VAL_INT(value) \
> > + { .kind = QNUM_I64, .u.i64 = value }
> > +#define QNUM_VAL_UINT(value) \
> > + { .kind = QNUM_U64, .u.u64 = value }
> > +#define QNUM_VAL_DOUBLE(value) \
> > + { .kind = QNUM_DOUBLE, .u.dbl = value }
> > +
> > +struct QNum {
> > + struct QObjectBase_ base;
> > + QNumValue value;
> > };
> >
> > +/**
> > + * qnum_from_int(): Create a new QNum from a QNumValue
> >
>
> Copy-pasta qnum_from_int() -> qnum_from_value()
Oops! I will fix it in v3, or submit a fixup patch if that's the
only needed change.
>
> I wonder if there is a check for that kind of mistake, as it may be common.
It looks like kernel-doc ignores what's before the colon in the
summary line. It probably shouldn't.
>
> + * @value: QNumValue
> > + *
> > + * Return strong reference.
> > + */
> > +QNum *qnum_from_value(QNumValue value);
> >
> +
> > QNum *qnum_from_int(int64_t value);
> > QNum *qnum_from_uint(uint64_t value);
> > QNum *qnum_from_double(double value);
> >
> > +/**
> > + * qnum_get_value(): Get QNumValue from QNum
> > + * @qn: QNum object
> > + */
> > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> > +{
> > + return &qn->value;
> > +}
> > +
> >
>
> Nothing uses that function in this patch. Should be put into use.
It is used in patch 5/8. Why do you think it's necessary to use
it in internal code too?
>
> bool qnum_get_try_int(const QNum *qn, int64_t *val);
> > int64_t qnum_get_int(const QNum *qn);
> >
> > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > index 69fd9a82d9..f80d4efd76 100644
> > --- a/qobject/qnum.c
> > +++ b/qobject/qnum.c
> > @@ -15,6 +15,15 @@
> > #include "qemu/osdep.h"
> > #include "qapi/qmp/qnum.h"
> >
> > +QNum *qnum_from_value(QNumValue value)
> >
>
> I wonder if it shouldn't be made "inline" in header too (if that can help
> avoid argument copy).
I'm unsure. I would make it inline (in a separate patch) if
there's evidence it's worth it. I expect the g_new() call to be
more expensive than any argument copying, though.
>
> +{
> > + QNum *qn = g_new(QNum, 1);
> > +
> > + qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > + qn->value = value;
> > + return qn;
> > +}
> > +
> > /**
> > * qnum_from_int(): Create a new QNum from an int64_t
> > * @value: int64_t value
> > @@ -23,13 +32,7 @@
> > */
> > QNum *qnum_from_int(int64_t value)
> > {
> > - QNum *qn = g_new(QNum, 1);
> > -
> > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > - qn->kind = QNUM_I64;
> > - qn->u.i64 = value;
> > -
> > - return qn;
> > + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> > }
> >
> > /**
> > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> > */
> > QNum *qnum_from_uint(uint64_t value)
> > {
> > - QNum *qn = g_new(QNum, 1);
> > -
> > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > - qn->kind = QNUM_U64;
> > - qn->u.u64 = value;
> > -
> > - return qn;
> > + return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> > }
> >
> > /**
> > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> > */
> > QNum *qnum_from_double(double value)
> > {
> > - QNum *qn = g_new(QNum, 1);
> > -
> > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > - qn->kind = QNUM_DOUBLE;
> > - qn->u.dbl = value;
> > -
> > - return qn;
> > + return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> > }
> >
> > /**
> > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> > */
> > bool qnum_get_try_int(const QNum *qn, int64_t *val)
> > {
> > - switch (qn->kind) {
> > + const QNumValue *qv = &qn->value;
> > +
> > + switch (qv->kind) {
> > case QNUM_I64:
> > - *val = qn->u.i64;
> > + *val = qv->u.i64;
> > return true;
> > case QNUM_U64:
> > - if (qn->u.u64 > INT64_MAX) {
> > + if (qv->u.u64 > INT64_MAX) {
> > return false;
> > }
> > - *val = qn->u.u64;
> > + *val = qv->u.u64;
> > return true;
> > case QNUM_DOUBLE:
> > return false;
> > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> > */
> > bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> > {
> > - switch (qn->kind) {
> > + const QNumValue *qv = &qn->value;
> > +
> > + switch (qv->kind) {
> > case QNUM_I64:
> > - if (qn->u.i64 < 0) {
> > + if (qv->u.i64 < 0) {
> > return false;
> > }
> > - *val = qn->u.i64;
> > + *val = qv->u.i64;
> > return true;
> > case QNUM_U64:
> > - *val = qn->u.u64;
> > + *val = qv->u.u64;
> > return true;
> > case QNUM_DOUBLE:
> > return false;
> > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> > */
> > double qnum_get_double(const QNum *qn)
> > {
> > - switch (qn->kind) {
> > + const QNumValue *qv = &qn->value;
> > +
> > + switch (qv->kind) {
> > case QNUM_I64:
> > - return qn->u.i64;
> > + return qv->u.i64;
> > case QNUM_U64:
> > - return qn->u.u64;
> > + return qv->u.u64;
> > case QNUM_DOUBLE:
> > - return qn->u.dbl;
> > + return qv->u.dbl;
> > }
> >
> > assert(0);
> > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
> >
> > char *qnum_to_string(QNum *qn)
> > {
> > + const QNumValue *qv = &qn->value;
> >
>
> qnum_get_value() ?
I prefer to not hide this behind a function call, in internal
code. But I don't mind changing it if you think it's important.
>
> char *buffer;
> > int len;
> >
> > - switch (qn->kind) {
> > + switch (qv->kind) {
> > case QNUM_I64:
> > - return g_strdup_printf("%" PRId64, qn->u.i64);
> > + return g_strdup_printf("%" PRId64, qv->u.i64);
> > case QNUM_U64:
> > - return g_strdup_printf("%" PRIu64, qn->u.u64);
> > + return g_strdup_printf("%" PRIu64, qv->u.u64);
> > case QNUM_DOUBLE:
> > /* FIXME: snprintf() is locale dependent; but JSON requires
> > * numbers to be formatted as if in the C locale. Dependence
> > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> > * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> > * and only rounding to a shorter number if the result would
> > * still produce the same floating point value. */
> > - buffer = g_strdup_printf("%f" , qn->u.dbl);
> > + buffer = g_strdup_printf("%f" , qv->u.dbl);
> > len = strlen(buffer);
> > while (len > 0 && buffer[len - 1] == '0') {
> > len--;
> > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> > */
> > bool qnum_is_equal(const QObject *x, const QObject *y)
> > {
> > - QNum *num_x = qobject_to(QNum, x);
> > - QNum *num_y = qobject_to(QNum, y);
> > + const QNum *qnum_x = qobject_to(QNum, x);
> > + const QNum *qnum_y = qobject_to(QNum, y);
> > + const QNumValue *num_x = &qnum_x->value;
> > + const QNumValue *num_y = &qnum_y->value;
> >
> > switch (num_x->kind) {
> > case QNUM_I64:
> > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> > index 4105015872..9499b0d845 100644
> > --- a/tests/check-qnum.c
> > +++ b/tests/check-qnum.c
> > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
> >
> > qn = qnum_from_int(value);
> > g_assert(qn != NULL);
> > - g_assert_cmpint(qn->kind, ==, QNUM_I64);
> > - g_assert_cmpint(qn->u.i64, ==, value);
> > + g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> > + g_assert_cmpint(qn->value.u.i64, ==, value);
> > g_assert_cmpint(qn->base.refcnt, ==, 1);
> > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >
> > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
> >
> > qn = qnum_from_uint(value);
> > g_assert(qn != NULL);
> > - g_assert_cmpint(qn->kind, ==, QNUM_U64);
> > - g_assert(qn->u.u64 == value);
> > + g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> > + g_assert(qn->value.u.u64 == value);
> > g_assert(qn->base.refcnt == 1);
> > g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
> >
> > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
> >
> > qn = qnum_from_double(value);
> > g_assert(qn != NULL);
> > - g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> > - g_assert_cmpfloat(qn->u.dbl, ==, value);
> > + g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> > + g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> > g_assert_cmpint(qn->base.refcnt, ==, 1);
> > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >
> > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> > const int64_t value = 0x1234567890abcdefLL;
> >
> > qn = qnum_from_int(value);
> > - g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> > + g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
> >
> > qobject_unref(qn);
> > }
> > --
> > 2.28.0
> >
> >
> >
> lgtm otherwise
Thanks!
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-17 14:42 ` Eduardo Habkost
@ 2020-11-17 14:58 ` Marc-André Lureau
2020-11-19 10:24 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 14:58 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 11862 bytes --]
On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> >
> > > Provide a separate QNumValue type that can be used for QNum value
> > > literals without the referencing counting and memory allocation
> > > features provided by QObject.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Changes v1 -> v2:
> > > * Fix "make check" failure, by updating check-qnum unit test to
> > > use the new struct fields
> > > ---
> > > include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> > > qobject/qnum.c | 78 ++++++++++++++++++++---------------------
> > > tests/check-qnum.c | 14 ++++----
> > > 3 files changed, 84 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > > index 55c27b1c24..62fbdfda68 100644
> > > --- a/include/qapi/qmp/qnum.h
> > > +++ b/include/qapi/qmp/qnum.h
> > > @@ -46,20 +46,56 @@ typedef enum {
> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> > > * convert under the hood.
> > > */
> > > -struct QNum {
> > > - struct QObjectBase_ base;
> > > +
> > > +/**
> > > + * struct QNumValue: the value of a QNum
> > > + *
> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> > > + */
> > > +typedef struct QNumValue {
> > > + /* private: */
> > > QNumKind kind;
> > > union {
> > > int64_t i64;
> > > uint64_t u64;
> > > double dbl;
> > > } u;
> > > +} QNumValue;
> > > +
> > > +#define QNUM_VAL_INT(value) \
> > > + { .kind = QNUM_I64, .u.i64 = value }
> > > +#define QNUM_VAL_UINT(value) \
> > > + { .kind = QNUM_U64, .u.u64 = value }
> > > +#define QNUM_VAL_DOUBLE(value) \
> > > + { .kind = QNUM_DOUBLE, .u.dbl = value }
> > > +
> > > +struct QNum {
> > > + struct QObjectBase_ base;
> > > + QNumValue value;
> > > };
> > >
> > > +/**
> > > + * qnum_from_int(): Create a new QNum from a QNumValue
> > >
> >
> > Copy-pasta qnum_from_int() -> qnum_from_value()
>
> Oops! I will fix it in v3, or submit a fixup patch if that's the
> only needed change.
>
> >
> > I wonder if there is a check for that kind of mistake, as it may be
> common.
>
> It looks like kernel-doc ignores what's before the colon in the
> summary line. It probably shouldn't.
>
> >
> > + * @value: QNumValue
> > > + *
> > > + * Return strong reference.
> > > + */
> > > +QNum *qnum_from_value(QNumValue value);
> > >
> > +
> > > QNum *qnum_from_int(int64_t value);
> > > QNum *qnum_from_uint(uint64_t value);
> > > QNum *qnum_from_double(double value);
> > >
> > > +/**
> > > + * qnum_get_value(): Get QNumValue from QNum
> > > + * @qn: QNum object
> > > + */
> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> > > +{
> > > + return &qn->value;
> > > +}
> > > +
> > >
> >
> > Nothing uses that function in this patch. Should be put into use.
>
> It is used in patch 5/8. Why do you think it's necessary to use
> it in internal code too?
>
Not necessarily, just want to make sure we don't introduce dead code. Fine
it's used later, perhaps worth noting in the commit message.
>
> >
> > bool qnum_get_try_int(const QNum *qn, int64_t *val);
> > > int64_t qnum_get_int(const QNum *qn);
> > >
> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > > index 69fd9a82d9..f80d4efd76 100644
> > > --- a/qobject/qnum.c
> > > +++ b/qobject/qnum.c
> > > @@ -15,6 +15,15 @@
> > > #include "qemu/osdep.h"
> > > #include "qapi/qmp/qnum.h"
> > >
> > > +QNum *qnum_from_value(QNumValue value)
> > >
> >
> > I wonder if it shouldn't be made "inline" in header too (if that can help
> > avoid argument copy).
>
> I'm unsure. I would make it inline (in a separate patch) if
> there's evidence it's worth it. I expect the g_new() call to be
> more expensive than any argument copying, though.
>
ok
> >
> > +{
> > > + QNum *qn = g_new(QNum, 1);
> > > +
> > > + qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > + qn->value = value;
> > > + return qn;
> > > +}
> > > +
> > > /**
> > > * qnum_from_int(): Create a new QNum from an int64_t
> > > * @value: int64_t value
> > > @@ -23,13 +32,7 @@
> > > */
> > > QNum *qnum_from_int(int64_t value)
> > > {
> > > - QNum *qn = g_new(QNum, 1);
> > > -
> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > - qn->kind = QNUM_I64;
> > > - qn->u.i64 = value;
> > > -
> > > - return qn;
> > > + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> > > }
> > >
> > > /**
> > > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> > > */
> > > QNum *qnum_from_uint(uint64_t value)
> > > {
> > > - QNum *qn = g_new(QNum, 1);
> > > -
> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > - qn->kind = QNUM_U64;
> > > - qn->u.u64 = value;
> > > -
> > > - return qn;
> > > + return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> > > }
> > >
> > > /**
> > > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> > > */
> > > QNum *qnum_from_double(double value)
> > > {
> > > - QNum *qn = g_new(QNum, 1);
> > > -
> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > > - qn->kind = QNUM_DOUBLE;
> > > - qn->u.dbl = value;
> > > -
> > > - return qn;
> > > + return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> > > }
> > >
> > > /**
> > > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> > > */
> > > bool qnum_get_try_int(const QNum *qn, int64_t *val)
> > > {
> > > - switch (qn->kind) {
> > > + const QNumValue *qv = &qn->value;
> > > +
> > > + switch (qv->kind) {
> > > case QNUM_I64:
> > > - *val = qn->u.i64;
> > > + *val = qv->u.i64;
> > > return true;
> > > case QNUM_U64:
> > > - if (qn->u.u64 > INT64_MAX) {
> > > + if (qv->u.u64 > INT64_MAX) {
> > > return false;
> > > }
> > > - *val = qn->u.u64;
> > > + *val = qv->u.u64;
> > > return true;
> > > case QNUM_DOUBLE:
> > > return false;
> > > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> > > */
> > > bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> > > {
> > > - switch (qn->kind) {
> > > + const QNumValue *qv = &qn->value;
> > > +
> > > + switch (qv->kind) {
> > > case QNUM_I64:
> > > - if (qn->u.i64 < 0) {
> > > + if (qv->u.i64 < 0) {
> > > return false;
> > > }
> > > - *val = qn->u.i64;
> > > + *val = qv->u.i64;
> > > return true;
> > > case QNUM_U64:
> > > - *val = qn->u.u64;
> > > + *val = qv->u.u64;
> > > return true;
> > > case QNUM_DOUBLE:
> > > return false;
> > > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> > > */
> > > double qnum_get_double(const QNum *qn)
> > > {
> > > - switch (qn->kind) {
> > > + const QNumValue *qv = &qn->value;
> > > +
> > > + switch (qv->kind) {
> > > case QNUM_I64:
> > > - return qn->u.i64;
> > > + return qv->u.i64;
> > > case QNUM_U64:
> > > - return qn->u.u64;
> > > + return qv->u.u64;
> > > case QNUM_DOUBLE:
> > > - return qn->u.dbl;
> > > + return qv->u.dbl;
> > > }
> > >
> > > assert(0);
> > > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
> > >
> > > char *qnum_to_string(QNum *qn)
> > > {
> > > + const QNumValue *qv = &qn->value;
> > >
> >
> > qnum_get_value() ?
>
> I prefer to not hide this behind a function call, in internal
> code. But I don't mind changing it if you think it's important.
>
no, it's ok to me
>
> >
> > char *buffer;
> > > int len;
> > >
> > > - switch (qn->kind) {
> > > + switch (qv->kind) {
> > > case QNUM_I64:
> > > - return g_strdup_printf("%" PRId64, qn->u.i64);
> > > + return g_strdup_printf("%" PRId64, qv->u.i64);
> > > case QNUM_U64:
> > > - return g_strdup_printf("%" PRIu64, qn->u.u64);
> > > + return g_strdup_printf("%" PRIu64, qv->u.u64);
> > > case QNUM_DOUBLE:
> > > /* FIXME: snprintf() is locale dependent; but JSON requires
> > > * numbers to be formatted as if in the C locale. Dependence
> > > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> > > * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> > > * and only rounding to a shorter number if the result would
> > > * still produce the same floating point value. */
> > > - buffer = g_strdup_printf("%f" , qn->u.dbl);
> > > + buffer = g_strdup_printf("%f" , qv->u.dbl);
> > > len = strlen(buffer);
> > > while (len > 0 && buffer[len - 1] == '0') {
> > > len--;
> > > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> > > */
> > > bool qnum_is_equal(const QObject *x, const QObject *y)
> > > {
> > > - QNum *num_x = qobject_to(QNum, x);
> > > - QNum *num_y = qobject_to(QNum, y);
> > > + const QNum *qnum_x = qobject_to(QNum, x);
> > > + const QNum *qnum_y = qobject_to(QNum, y);
> > > + const QNumValue *num_x = &qnum_x->value;
> > > + const QNumValue *num_y = &qnum_y->value;
> > >
> > > switch (num_x->kind) {
> > > case QNUM_I64:
> > > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> > > index 4105015872..9499b0d845 100644
> > > --- a/tests/check-qnum.c
> > > +++ b/tests/check-qnum.c
> > > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
> > >
> > > qn = qnum_from_int(value);
> > > g_assert(qn != NULL);
> > > - g_assert_cmpint(qn->kind, ==, QNUM_I64);
> > > - g_assert_cmpint(qn->u.i64, ==, value);
> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> > > + g_assert_cmpint(qn->value.u.i64, ==, value);
> > > g_assert_cmpint(qn->base.refcnt, ==, 1);
> > > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> > >
> > > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
> > >
> > > qn = qnum_from_uint(value);
> > > g_assert(qn != NULL);
> > > - g_assert_cmpint(qn->kind, ==, QNUM_U64);
> > > - g_assert(qn->u.u64 == value);
> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> > > + g_assert(qn->value.u.u64 == value);
> > > g_assert(qn->base.refcnt == 1);
> > > g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
> > >
> > > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
> > >
> > > qn = qnum_from_double(value);
> > > g_assert(qn != NULL);
> > > - g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> > > - g_assert_cmpfloat(qn->u.dbl, ==, value);
> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> > > + g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> > > g_assert_cmpint(qn->base.refcnt, ==, 1);
> > > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> > >
> > > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> > > const int64_t value = 0x1234567890abcdefLL;
> > >
> > > qn = qnum_from_int(value);
> > > - g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> > > + g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
> > >
> > > qobject_unref(qn);
> > > }
> > > --
> > > 2.28.0
> > >
> > >
> > >
> > lgtm otherwise
>
> Thanks!
>
> --
> Eduardo
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 16360 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-17 14:58 ` Marc-André Lureau
@ 2020-11-19 10:24 ` Markus Armbruster
2020-11-19 18:21 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-19 10:24 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, QEMU
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
>> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
>> wrote:
>> >
>> > > Provide a separate QNumValue type that can be used for QNum value
>> > > literals without the referencing counting and memory allocation
>> > > features provided by QObject.
>> > >
>> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > > ---
>> > > Changes v1 -> v2:
>> > > * Fix "make check" failure, by updating check-qnum unit test to
>> > > use the new struct fields
>> > > ---
>> > > include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
>> > > qobject/qnum.c | 78 ++++++++++++++++++++---------------------
>> > > tests/check-qnum.c | 14 ++++----
>> > > 3 files changed, 84 insertions(+), 48 deletions(-)
>> > >
>> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
>> > > index 55c27b1c24..62fbdfda68 100644
>> > > --- a/include/qapi/qmp/qnum.h
>> > > +++ b/include/qapi/qmp/qnum.h
>> > > @@ -46,20 +46,56 @@ typedef enum {
>> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
>> > > * convert under the hood.
>> > > */
>> > > -struct QNum {
>> > > - struct QObjectBase_ base;
>> > > +
>> > > +/**
>> > > + * struct QNumValue: the value of a QNum
>> > > + *
>> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
>> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
>> > > + */
>> > > +typedef struct QNumValue {
>> > > + /* private: */
Do we care?
>> > > QNumKind kind;
>> > > union {
>> > > int64_t i64;
>> > > uint64_t u64;
>> > > double dbl;
>> > > } u;
>> > > +} QNumValue;
>> > > +
>> > > +#define QNUM_VAL_INT(value) \
>> > > + { .kind = QNUM_I64, .u.i64 = value }
>> > > +#define QNUM_VAL_UINT(value) \
>> > > + { .kind = QNUM_U64, .u.u64 = value }
>> > > +#define QNUM_VAL_DOUBLE(value) \
>> > > + { .kind = QNUM_DOUBLE, .u.dbl = value }
>> > > +
>> > > +struct QNum {
>> > > + struct QObjectBase_ base;
>> > > + QNumValue value;
>> > > };
>> > >
>> > > +/**
>> > > + * qnum_from_int(): Create a new QNum from a QNumValue
>> > >
>> >
>> > Copy-pasta qnum_from_int() -> qnum_from_value()
>>
>> Oops! I will fix it in v3, or submit a fixup patch if that's the
>> only needed change.
>>
>> >
>> > I wonder if there is a check for that kind of mistake, as it may be
>> common.
>>
>> It looks like kernel-doc ignores what's before the colon in the
>> summary line. It probably shouldn't.
>>
>> >
>> > + * @value: QNumValue
>> > > + *
>> > > + * Return strong reference.
>> > > + */
>> > > +QNum *qnum_from_value(QNumValue value);
>> > >
>> > +
>> > > QNum *qnum_from_int(int64_t value);
>> > > QNum *qnum_from_uint(uint64_t value);
>> > > QNum *qnum_from_double(double value);
>> > >
>> > > +/**
>> > > + * qnum_get_value(): Get QNumValue from QNum
>> > > + * @qn: QNum object
>> > > + */
>> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
>> > > +{
>> > > + return &qn->value;
>> > > +}
>> > > +
>> > >
>> >
>> > Nothing uses that function in this patch. Should be put into use.
>>
>> It is used in patch 5/8. Why do you think it's necessary to use
>> it in internal code too?
>>
>
> Not necessarily, just want to make sure we don't introduce dead code. Fine
> it's used later, perhaps worth noting in the commit message.
Yes, use later in the same series suffices.
The whole patch makes sense only because later patches put QNumValue to
use. The commit message could perhaps be more explicit about that. No
need to be explicit about every new function, though.
That said, I wouldn't bother with a getter function. Yes, we have
similar trivial getters for other QFoo. I don't care for them, either.
The actual structure of these data types is trivial, and not worth
hiding.
Also, I'm wary of inline functions in headers. *Especially* when they
require additional #include (which this one doesn't). I accept them
when they provide a worthwhile performance improvement. Guesses don't
count as evidence :)
>>
>> >
>> > bool qnum_get_try_int(const QNum *qn, int64_t *val);
>> > > int64_t qnum_get_int(const QNum *qn);
>> > >
>> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
>> > > index 69fd9a82d9..f80d4efd76 100644
>> > > --- a/qobject/qnum.c
>> > > +++ b/qobject/qnum.c
>> > > @@ -15,6 +15,15 @@
>> > > #include "qemu/osdep.h"
>> > > #include "qapi/qmp/qnum.h"
>> > >
>> > > +QNum *qnum_from_value(QNumValue value)
>> > >
>> >
>> > I wonder if it shouldn't be made "inline" in header too (if that can help
>> > avoid argument copy).
>>
>> I'm unsure. I would make it inline (in a separate patch) if
>> there's evidence it's worth it. I expect the g_new() call to be
>> more expensive than any argument copying, though.
>
> ok
I'm with Eduardo here.
>> >
>> > +{
>> > > + QNum *qn = g_new(QNum, 1);
>> > > +
>> > > + qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > + qn->value = value;
>> > > + return qn;
>> > > +}
>> > > +
>> > > /**
>> > > * qnum_from_int(): Create a new QNum from an int64_t
>> > > * @value: int64_t value
>> > > @@ -23,13 +32,7 @@
>> > > */
>> > > QNum *qnum_from_int(int64_t value)
>> > > {
>> > > - QNum *qn = g_new(QNum, 1);
>> > > -
>> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > - qn->kind = QNUM_I64;
>> > > - qn->u.i64 = value;
>> > > -
>> > > - return qn;
>> > > + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
No space between between (type) and its operand, please.
Could we lift the cast into the macro somehow?
>> > > }
>> > >
>> > > /**
>> > > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
>> > > */
>> > > QNum *qnum_from_uint(uint64_t value)
>> > > {
>> > > - QNum *qn = g_new(QNum, 1);
>> > > -
>> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > - qn->kind = QNUM_U64;
>> > > - qn->u.u64 = value;
>> > > -
>> > > - return qn;
>> > > + return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
>> > > }
>> > >
>> > > /**
>> > > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
>> > > */
>> > > QNum *qnum_from_double(double value)
>> > > {
>> > > - QNum *qn = g_new(QNum, 1);
>> > > -
>> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
>> > > - qn->kind = QNUM_DOUBLE;
>> > > - qn->u.dbl = value;
>> > > -
>> > > - return qn;
>> > > + return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
>> > > }
>> > >
>> > > /**
>> > > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
>> > > */
>> > > bool qnum_get_try_int(const QNum *qn, int64_t *val)
>> > > {
>> > > - switch (qn->kind) {
>> > > + const QNumValue *qv = &qn->value;
>> > > +
>> > > + switch (qv->kind) {
>> > > case QNUM_I64:
>> > > - *val = qn->u.i64;
>> > > + *val = qv->u.i64;
>> > > return true;
>> > > case QNUM_U64:
>> > > - if (qn->u.u64 > INT64_MAX) {
>> > > + if (qv->u.u64 > INT64_MAX) {
>> > > return false;
>> > > }
>> > > - *val = qn->u.u64;
>> > > + *val = qv->u.u64;
>> > > return true;
>> > > case QNUM_DOUBLE:
>> > > return false;
Here you add a new variable to shorten "qn->value." to "qv->". In
tests/check-qnum.c you don't. I'm not sure the variable is worthwhile.
>> > > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
>> > > */
>> > > bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
>> > > {
>> > > - switch (qn->kind) {
>> > > + const QNumValue *qv = &qn->value;
>> > > +
>> > > + switch (qv->kind) {
>> > > case QNUM_I64:
>> > > - if (qn->u.i64 < 0) {
>> > > + if (qv->u.i64 < 0) {
>> > > return false;
>> > > }
>> > > - *val = qn->u.i64;
>> > > + *val = qv->u.i64;
>> > > return true;
>> > > case QNUM_U64:
>> > > - *val = qn->u.u64;
>> > > + *val = qv->u.u64;
>> > > return true;
>> > > case QNUM_DOUBLE:
>> > > return false;
>> > > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
>> > > */
>> > > double qnum_get_double(const QNum *qn)
>> > > {
>> > > - switch (qn->kind) {
>> > > + const QNumValue *qv = &qn->value;
>> > > +
>> > > + switch (qv->kind) {
>> > > case QNUM_I64:
>> > > - return qn->u.i64;
>> > > + return qv->u.i64;
>> > > case QNUM_U64:
>> > > - return qn->u.u64;
>> > > + return qv->u.u64;
>> > > case QNUM_DOUBLE:
>> > > - return qn->u.dbl;
>> > > + return qv->u.dbl;
>> > > }
>> > >
>> > > assert(0);
>> > > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
>> > >
>> > > char *qnum_to_string(QNum *qn)
>> > > {
>> > > + const QNumValue *qv = &qn->value;
>> > >
>> >
>> > qnum_get_value() ?
>>
>> I prefer to not hide this behind a function call, in internal
>> code. But I don't mind changing it if you think it's important.
Me too. Even in external code.
> no, it's ok to me
>
>
>>
>> >
>> > char *buffer;
>> > > int len;
>> > >
>> > > - switch (qn->kind) {
>> > > + switch (qv->kind) {
>> > > case QNUM_I64:
>> > > - return g_strdup_printf("%" PRId64, qn->u.i64);
>> > > + return g_strdup_printf("%" PRId64, qv->u.i64);
>> > > case QNUM_U64:
>> > > - return g_strdup_printf("%" PRIu64, qn->u.u64);
>> > > + return g_strdup_printf("%" PRIu64, qv->u.u64);
>> > > case QNUM_DOUBLE:
>> > > /* FIXME: snprintf() is locale dependent; but JSON requires
>> > > * numbers to be formatted as if in the C locale. Dependence
>> > > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
>> > > * rounding errors; we should be using DBL_DECIMAL_DIG (17),
>> > > * and only rounding to a shorter number if the result would
>> > > * still produce the same floating point value. */
>> > > - buffer = g_strdup_printf("%f" , qn->u.dbl);
>> > > + buffer = g_strdup_printf("%f" , qv->u.dbl);
>> > > len = strlen(buffer);
>> > > while (len > 0 && buffer[len - 1] == '0') {
>> > > len--;
>> > > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
>> > > */
>> > > bool qnum_is_equal(const QObject *x, const QObject *y)
>> > > {
>> > > - QNum *num_x = qobject_to(QNum, x);
>> > > - QNum *num_y = qobject_to(QNum, y);
>> > > + const QNum *qnum_x = qobject_to(QNum, x);
>> > > + const QNum *qnum_y = qobject_to(QNum, y);
A bit of drive-by constification. Okay.
>> > > + const QNumValue *num_x = &qnum_x->value;
>> > > + const QNumValue *num_y = &qnum_y->value;
>> > >
>> > > switch (num_x->kind) {
>> > > case QNUM_I64:
>> > > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
>> > > index 4105015872..9499b0d845 100644
>> > > --- a/tests/check-qnum.c
>> > > +++ b/tests/check-qnum.c
>> > > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
>> > >
>> > > qn = qnum_from_int(value);
>> > > g_assert(qn != NULL);
>> > > - g_assert_cmpint(qn->kind, ==, QNUM_I64);
>> > > - g_assert_cmpint(qn->u.i64, ==, value);
>> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
>> > > + g_assert_cmpint(qn->value.u.i64, ==, value);
>> > > g_assert_cmpint(qn->base.refcnt, ==, 1);
>> > > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>> > >
>> > > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
>> > >
>> > > qn = qnum_from_uint(value);
>> > > g_assert(qn != NULL);
>> > > - g_assert_cmpint(qn->kind, ==, QNUM_U64);
>> > > - g_assert(qn->u.u64 == value);
>> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
>> > > + g_assert(qn->value.u.u64 == value);
>> > > g_assert(qn->base.refcnt == 1);
>> > > g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
>> > >
>> > > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
>> > >
>> > > qn = qnum_from_double(value);
>> > > g_assert(qn != NULL);
>> > > - g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
>> > > - g_assert_cmpfloat(qn->u.dbl, ==, value);
>> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
>> > > + g_assert_cmpfloat(qn->value.u.dbl, ==, value);
>> > > g_assert_cmpint(qn->base.refcnt, ==, 1);
>> > > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
>> > >
>> > > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
>> > > const int64_t value = 0x1234567890abcdefLL;
>> > >
>> > > qn = qnum_from_int(value);
>> > > - g_assert_cmpint((int64_t) qn->u.i64, ==, value);
>> > > + g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
>> > >
>> > > qobject_unref(qn);
>> > > }
>> > > --
>> > > 2.28.0
>> > >
>> > >
>> > >
>> > lgtm otherwise
>>
>> Thanks!
>>
>> --
>> Eduardo
>>
>>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-19 10:24 ` Markus Armbruster
@ 2020-11-19 18:21 ` Eduardo Habkost
2020-11-19 20:55 ` Eduardo Habkost
2020-11-20 5:29 ` Markus Armbruster
0 siblings, 2 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 18:21 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> >> wrote:
> >> >
> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> > > literals without the referencing counting and memory allocation
> >> > > features provided by QObject.
> >> > >
> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > > ---
> >> > > Changes v1 -> v2:
> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> > > use the new struct fields
> >> > > ---
> >> > > include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> >> > > qobject/qnum.c | 78 ++++++++++++++++++++---------------------
> >> > > tests/check-qnum.c | 14 ++++----
> >> > > 3 files changed, 84 insertions(+), 48 deletions(-)
> >> > >
> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> > > index 55c27b1c24..62fbdfda68 100644
> >> > > --- a/include/qapi/qmp/qnum.h
> >> > > +++ b/include/qapi/qmp/qnum.h
> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> >> > > * convert under the hood.
> >> > > */
> >> > > -struct QNum {
> >> > > - struct QObjectBase_ base;
> >> > > +
> >> > > +/**
> >> > > + * struct QNumValue: the value of a QNum
> >> > > + *
> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> > > + */
> >> > > +typedef struct QNumValue {
> >> > > + /* private: */
>
> Do we care?
Are you asking if we want to make them private, or if we want to
document them as private (assuming they are).
The answer to the latter is yes ("private:" is an indication to
kernel-doc). The answer to the former seems to be "no", based on
your other comments.
Or maybe `kind` should be public and `u` should be private?
>
> >> > > QNumKind kind;
> >> > > union {
> >> > > int64_t i64;
> >> > > uint64_t u64;
> >> > > double dbl;
> >> > > } u;
> >> > > +} QNumValue;
> >> > > +
> >> > > +#define QNUM_VAL_INT(value) \
> >> > > + { .kind = QNUM_I64, .u.i64 = value }
> >> > > +#define QNUM_VAL_UINT(value) \
> >> > > + { .kind = QNUM_U64, .u.u64 = value }
> >> > > +#define QNUM_VAL_DOUBLE(value) \
> >> > > + { .kind = QNUM_DOUBLE, .u.dbl = value }
> >> > > +
> >> > > +struct QNum {
> >> > > + struct QObjectBase_ base;
> >> > > + QNumValue value;
> >> > > };
> >> > >
> >> > > +/**
> >> > > + * qnum_from_int(): Create a new QNum from a QNumValue
> >> > >
> >> >
> >> > Copy-pasta qnum_from_int() -> qnum_from_value()
> >>
> >> Oops! I will fix it in v3, or submit a fixup patch if that's the
> >> only needed change.
> >>
> >> >
> >> > I wonder if there is a check for that kind of mistake, as it may be
> >> common.
> >>
> >> It looks like kernel-doc ignores what's before the colon in the
> >> summary line. It probably shouldn't.
> >>
> >> >
> >> > + * @value: QNumValue
> >> > > + *
> >> > > + * Return strong reference.
> >> > > + */
> >> > > +QNum *qnum_from_value(QNumValue value);
> >> > >
> >> > +
> >> > > QNum *qnum_from_int(int64_t value);
> >> > > QNum *qnum_from_uint(uint64_t value);
> >> > > QNum *qnum_from_double(double value);
> >> > >
> >> > > +/**
> >> > > + * qnum_get_value(): Get QNumValue from QNum
> >> > > + * @qn: QNum object
> >> > > + */
> >> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> >> > > +{
> >> > > + return &qn->value;
> >> > > +}
> >> > > +
> >> > >
> >> >
> >> > Nothing uses that function in this patch. Should be put into use.
> >>
> >> It is used in patch 5/8. Why do you think it's necessary to use
> >> it in internal code too?
> >>
> >
> > Not necessarily, just want to make sure we don't introduce dead code. Fine
> > it's used later, perhaps worth noting in the commit message.
>
> Yes, use later in the same series suffices.
>
> The whole patch makes sense only because later patches put QNumValue to
> use. The commit message could perhaps be more explicit about that. No
> need to be explicit about every new function, though.
>
> That said, I wouldn't bother with a getter function. Yes, we have
> similar trivial getters for other QFoo. I don't care for them, either.
> The actual structure of these data types is trivial, and not worth
> hiding.
>
> Also, I'm wary of inline functions in headers. *Especially* when they
> require additional #include (which this one doesn't). I accept them
> when they provide a worthwhile performance improvement. Guesses don't
> count as evidence :)
>
> >>
> >> >
> >> > bool qnum_get_try_int(const QNum *qn, int64_t *val);
> >> > > int64_t qnum_get_int(const QNum *qn);
> >> > >
> >> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> >> > > index 69fd9a82d9..f80d4efd76 100644
> >> > > --- a/qobject/qnum.c
> >> > > +++ b/qobject/qnum.c
> >> > > @@ -15,6 +15,15 @@
> >> > > #include "qemu/osdep.h"
> >> > > #include "qapi/qmp/qnum.h"
> >> > >
> >> > > +QNum *qnum_from_value(QNumValue value)
> >> > >
> >> >
> >> > I wonder if it shouldn't be made "inline" in header too (if that can help
> >> > avoid argument copy).
> >>
> >> I'm unsure. I would make it inline (in a separate patch) if
> >> there's evidence it's worth it. I expect the g_new() call to be
> >> more expensive than any argument copying, though.
> >
> > ok
>
> I'm with Eduardo here.
>
> >> >
> >> > +{
> >> > > + QNum *qn = g_new(QNum, 1);
> >> > > +
> >> > > + qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > + qn->value = value;
> >> > > + return qn;
> >> > > +}
> >> > > +
> >> > > /**
> >> > > * qnum_from_int(): Create a new QNum from an int64_t
> >> > > * @value: int64_t value
> >> > > @@ -23,13 +32,7 @@
> >> > > */
> >> > > QNum *qnum_from_int(int64_t value)
> >> > > {
> >> > > - QNum *qn = g_new(QNum, 1);
> >> > > -
> >> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > - qn->kind = QNUM_I64;
> >> > > - qn->u.i64 = value;
> >> > > -
> >> > > - return qn;
> >> > > + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
>
> No space between between (type) and its operand, please.
>
> Could we lift the cast into the macro somehow?
I think we can. I had thought the cast in the macro would break
usage as static variable initializers. I was wrong.
>
> >> > > }
> >> > >
> >> > > /**
> >> > > @@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
> >> > > */
> >> > > QNum *qnum_from_uint(uint64_t value)
> >> > > {
> >> > > - QNum *qn = g_new(QNum, 1);
> >> > > -
> >> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > - qn->kind = QNUM_U64;
> >> > > - qn->u.u64 = value;
> >> > > -
> >> > > - return qn;
> >> > > + return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
> >> > > }
> >> > >
> >> > > /**
> >> > > @@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
> >> > > */
> >> > > QNum *qnum_from_double(double value)
> >> > > {
> >> > > - QNum *qn = g_new(QNum, 1);
> >> > > -
> >> > > - qobject_init(QOBJECT(qn), QTYPE_QNUM);
> >> > > - qn->kind = QNUM_DOUBLE;
> >> > > - qn->u.dbl = value;
> >> > > -
> >> > > - return qn;
> >> > > + return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
> >> > > }
> >> > >
> >> > > /**
> >> > > @@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
> >> > > */
> >> > > bool qnum_get_try_int(const QNum *qn, int64_t *val)
> >> > > {
> >> > > - switch (qn->kind) {
> >> > > + const QNumValue *qv = &qn->value;
> >> > > +
> >> > > + switch (qv->kind) {
> >> > > case QNUM_I64:
> >> > > - *val = qn->u.i64;
> >> > > + *val = qv->u.i64;
> >> > > return true;
> >> > > case QNUM_U64:
> >> > > - if (qn->u.u64 > INT64_MAX) {
> >> > > + if (qv->u.u64 > INT64_MAX) {
> >> > > return false;
> >> > > }
> >> > > - *val = qn->u.u64;
> >> > > + *val = qv->u.u64;
> >> > > return true;
> >> > > case QNUM_DOUBLE:
> >> > > return false;
>
> Here you add a new variable to shorten "qn->value." to "qv->". In
> tests/check-qnum.c you don't. I'm not sure the variable is worthwhile.
The variable is worthwhile if it becomes a parameter to
qnum_value_get_try_int().
If we don't create a qnum_value_get_try_int() function, I still
think the variable makes the code easier to read, but I'm not too
attached to it.
>
> >> > > @@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
> >> > > */
> >> > > bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
> >> > > {
> >> > > - switch (qn->kind) {
> >> > > + const QNumValue *qv = &qn->value;
> >> > > +
> >> > > + switch (qv->kind) {
> >> > > case QNUM_I64:
> >> > > - if (qn->u.i64 < 0) {
> >> > > + if (qv->u.i64 < 0) {
> >> > > return false;
> >> > > }
> >> > > - *val = qn->u.i64;
> >> > > + *val = qv->u.i64;
> >> > > return true;
> >> > > case QNUM_U64:
> >> > > - *val = qn->u.u64;
> >> > > + *val = qv->u.u64;
> >> > > return true;
> >> > > case QNUM_DOUBLE:
> >> > > return false;
> >> > > @@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
> >> > > */
> >> > > double qnum_get_double(const QNum *qn)
> >> > > {
> >> > > - switch (qn->kind) {
> >> > > + const QNumValue *qv = &qn->value;
> >> > > +
> >> > > + switch (qv->kind) {
> >> > > case QNUM_I64:
> >> > > - return qn->u.i64;
> >> > > + return qv->u.i64;
> >> > > case QNUM_U64:
> >> > > - return qn->u.u64;
> >> > > + return qv->u.u64;
> >> > > case QNUM_DOUBLE:
> >> > > - return qn->u.dbl;
> >> > > + return qv->u.dbl;
> >> > > }
> >> > >
> >> > > assert(0);
> >> > > @@ -171,14 +168,15 @@ double qnum_get_double(const QNum *qn)
> >> > >
> >> > > char *qnum_to_string(QNum *qn)
> >> > > {
> >> > > + const QNumValue *qv = &qn->value;
> >> > >
> >> >
> >> > qnum_get_value() ?
> >>
> >> I prefer to not hide this behind a function call, in internal
> >> code. But I don't mind changing it if you think it's important.
>
> Me too. Even in external code.
Understood.
>
> > no, it's ok to me
> >
> >
> >>
> >> >
> >> > char *buffer;
> >> > > int len;
> >> > >
> >> > > - switch (qn->kind) {
> >> > > + switch (qv->kind) {
> >> > > case QNUM_I64:
> >> > > - return g_strdup_printf("%" PRId64, qn->u.i64);
> >> > > + return g_strdup_printf("%" PRId64, qv->u.i64);
> >> > > case QNUM_U64:
> >> > > - return g_strdup_printf("%" PRIu64, qn->u.u64);
> >> > > + return g_strdup_printf("%" PRIu64, qv->u.u64);
> >> > > case QNUM_DOUBLE:
> >> > > /* FIXME: snprintf() is locale dependent; but JSON requires
> >> > > * numbers to be formatted as if in the C locale. Dependence
> >> > > @@ -189,7 +187,7 @@ char *qnum_to_string(QNum *qn)
> >> > > * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> >> > > * and only rounding to a shorter number if the result would
> >> > > * still produce the same floating point value. */
> >> > > - buffer = g_strdup_printf("%f" , qn->u.dbl);
> >> > > + buffer = g_strdup_printf("%f" , qv->u.dbl);
> >> > > len = strlen(buffer);
> >> > > while (len > 0 && buffer[len - 1] == '0') {
> >> > > len--;
> >> > > @@ -221,8 +219,10 @@ char *qnum_to_string(QNum *qn)
> >> > > */
> >> > > bool qnum_is_equal(const QObject *x, const QObject *y)
> >> > > {
> >> > > - QNum *num_x = qobject_to(QNum, x);
> >> > > - QNum *num_y = qobject_to(QNum, y);
> >> > > + const QNum *qnum_x = qobject_to(QNum, x);
> >> > > + const QNum *qnum_y = qobject_to(QNum, y);
>
> A bit of drive-by constification. Okay.
In my defense, I will argue that those are four new variables.
the variables `QNum *num_x, *num_y` don't exist anymore.
Anyway, I will probably keep the `QNumValue *num_x, *num_y`
variables untouched in the next version, and choose another name
for the new QNumValue variables (which will become
qnum_value_is_equal() parameters in the next patch).
>
> >> > > + const QNumValue *num_x = &qnum_x->value;
> >> > > + const QNumValue *num_y = &qnum_y->value;
> >> > >
> >> > > switch (num_x->kind) {
> >> > > case QNUM_I64:
> >> > > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> >> > > index 4105015872..9499b0d845 100644
> >> > > --- a/tests/check-qnum.c
> >> > > +++ b/tests/check-qnum.c
> >> > > @@ -30,8 +30,8 @@ static void qnum_from_int_test(void)
> >> > >
> >> > > qn = qnum_from_int(value);
> >> > > g_assert(qn != NULL);
> >> > > - g_assert_cmpint(qn->kind, ==, QNUM_I64);
> >> > > - g_assert_cmpint(qn->u.i64, ==, value);
> >> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_I64);
> >> > > + g_assert_cmpint(qn->value.u.i64, ==, value);
> >> > > g_assert_cmpint(qn->base.refcnt, ==, 1);
> >> > > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >> > >
> >> > > @@ -45,8 +45,8 @@ static void qnum_from_uint_test(void)
> >> > >
> >> > > qn = qnum_from_uint(value);
> >> > > g_assert(qn != NULL);
> >> > > - g_assert_cmpint(qn->kind, ==, QNUM_U64);
> >> > > - g_assert(qn->u.u64 == value);
> >> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_U64);
> >> > > + g_assert(qn->value.u.u64 == value);
> >> > > g_assert(qn->base.refcnt == 1);
> >> > > g_assert(qobject_type(QOBJECT(qn)) == QTYPE_QNUM);
> >> > >
> >> > > @@ -60,8 +60,8 @@ static void qnum_from_double_test(void)
> >> > >
> >> > > qn = qnum_from_double(value);
> >> > > g_assert(qn != NULL);
> >> > > - g_assert_cmpint(qn->kind, ==, QNUM_DOUBLE);
> >> > > - g_assert_cmpfloat(qn->u.dbl, ==, value);
> >> > > + g_assert_cmpint(qn->value.kind, ==, QNUM_DOUBLE);
> >> > > + g_assert_cmpfloat(qn->value.u.dbl, ==, value);
> >> > > g_assert_cmpint(qn->base.refcnt, ==, 1);
> >> > > g_assert_cmpint(qobject_type(QOBJECT(qn)), ==, QTYPE_QNUM);
> >> > >
> >> > > @@ -74,7 +74,7 @@ static void qnum_from_int64_test(void)
> >> > > const int64_t value = 0x1234567890abcdefLL;
> >> > >
> >> > > qn = qnum_from_int(value);
> >> > > - g_assert_cmpint((int64_t) qn->u.i64, ==, value);
> >> > > + g_assert_cmpint((int64_t) qn->value.u.i64, ==, value);
> >> > >
> >> > > qobject_unref(qn);
> >> > > }
> >> > > --
> >> > > 2.28.0
> >> > >
> >> > >
> >> > >
> >> > lgtm otherwise
> >>
> >> Thanks!
> >>
> >> --
> >> Eduardo
> >>
> >>
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-19 18:21 ` Eduardo Habkost
@ 2020-11-19 20:55 ` Eduardo Habkost
2020-11-20 9:05 ` Markus Armbruster
2020-11-20 5:29 ` Markus Armbruster
1 sibling, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 20:55 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Thu, Nov 19, 2020 at 01:21:58PM -0500, Eduardo Habkost wrote:
> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
[...]
> > >> > > + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> >
> > No space between between (type) and its operand, please.
> >
> > Could we lift the cast into the macro somehow?
>
> I think we can. I had thought the cast in the macro would break
> usage as static variable initializers. I was wrong.
Actually, including the cast in the macro breaks QLIT_QDICT
initializers (which use (QLitDictEntry[]) compound literals), and
I don't know why.
Compound literals in initializers of static variables is a GCC
extension. I don't understand why it doesn't work inside array
compound literals, though.
Any language lawyers around?
This works:
typedef struct QLit {
int x, y;
} QLit;
typedef struct Entry {
int key;
QLit value;
} Entry;
Entry e = { .key = 0, .value = (QLit) { 1, 2 } };
This works:
Entry *es1 = (Entry[]) {
{ .key = 0, .value = { 1, 2 } },
};
But this doesn't:
Entry *es2 = (Entry[]) {
{ .key = 0, .value = (QLit) { 1, 2 } },
};
dict.c:16:24: error: initializer element is not constant
16 | Entry *es2 = (Entry[]) {
| ^
dict.c:16:24: note: (near initialization for ‘es2’)
(gcc (GCC) 10.2.1 20201005 (Red Hat 10.2.1-5))
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-19 20:55 ` Eduardo Habkost
@ 2020-11-20 9:05 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2020-11-20 9:05 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Nov 19, 2020 at 01:21:58PM -0500, Eduardo Habkost wrote:
>> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> [...]
>> > >> > > + return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
>> >
>> > No space between between (type) and its operand, please.
>> >
>> > Could we lift the cast into the macro somehow?
>>
>> I think we can. I had thought the cast in the macro would break
>> usage as static variable initializers. I was wrong.
>
> Actually, including the cast in the macro breaks QLIT_QDICT
> initializers (which use (QLitDictEntry[]) compound literals), and
> I don't know why.
>
> Compound literals in initializers of static variables is a GCC
> extension. I don't understand why it doesn't work inside array
> compound literals, though.
>
> Any language lawyers around?
>
> This works:
>
> typedef struct QLit {
> int x, y;
> } QLit;
>
> typedef struct Entry {
> int key;
> QLit value;
> } Entry;
>
> Entry e = { .key = 0, .value = (QLit) { 1, 2 } };
>
> This works:
>
> Entry *es1 = (Entry[]) {
> { .key = 0, .value = { 1, 2 } },
> };
>
> But this doesn't:
>
> Entry *es2 = (Entry[]) {
> { .key = 0, .value = (QLit) { 1, 2 } },
> };
>
> dict.c:16:24: error: initializer element is not constant
> 16 | Entry *es2 = (Entry[]) {
> | ^
> dict.c:16:24: note: (near initialization for ‘es2’)
>
> (gcc (GCC) 10.2.1 20201005 (Red Hat 10.2.1-5))
Can't explain this offhand.
Another pecularity: a const QLitObject is for the most part not actually
const. Evidence:
$ size bld-x86/libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-introspect.c.o
text data bss dec hex filename
19590 351600 48 371238 5aa26 bld-x86/libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-introspect.c.o
Score 5 out of 100 points.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-19 18:21 ` Eduardo Habkost
2020-11-19 20:55 ` Eduardo Habkost
@ 2020-11-20 5:29 ` Markus Armbruster
2020-11-20 18:27 ` Eduardo Habkost
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-20 5:29 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >
>> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
>> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
>> >> wrote:
>> >> >
>> >> > > Provide a separate QNumValue type that can be used for QNum value
>> >> > > literals without the referencing counting and memory allocation
>> >> > > features provided by QObject.
>> >> > >
>> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> >> > > ---
>> >> > > Changes v1 -> v2:
>> >> > > * Fix "make check" failure, by updating check-qnum unit test to
>> >> > > use the new struct fields
>> >> > > ---
>> >> > > include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
>> >> > > qobject/qnum.c | 78 ++++++++++++++++++++---------------------
>> >> > > tests/check-qnum.c | 14 ++++----
>> >> > > 3 files changed, 84 insertions(+), 48 deletions(-)
>> >> > >
>> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
>> >> > > index 55c27b1c24..62fbdfda68 100644
>> >> > > --- a/include/qapi/qmp/qnum.h
>> >> > > +++ b/include/qapi/qmp/qnum.h
>> >> > > @@ -46,20 +46,56 @@ typedef enum {
>> >> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
>> >> > > * convert under the hood.
>> >> > > */
>> >> > > -struct QNum {
>> >> > > - struct QObjectBase_ base;
>> >> > > +
>> >> > > +/**
>> >> > > + * struct QNumValue: the value of a QNum
>> >> > > + *
>> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
>> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
>> >> > > + */
>> >> > > +typedef struct QNumValue {
>> >> > > + /* private: */
>>
>> Do we care?
>
> Are you asking if we want to make them private, or if we want to
> document them as private (assuming they are).
>
> The answer to the latter is yes ("private:" is an indication to
> kernel-doc). The answer to the former seems to be "no", based on
> your other comments.
>
> Or maybe `kind` should be public and `u` should be private?
You're factoring out a part of struct QNum into new struct QNumValue.
struct QNum is not private before the patch. I see no need to start
making it or parts of it private now.
When the structure of a data type is to be kept away from its users, I
prefer to keep it out of the public header, so the compiler enforces the
encapsulation.
[...]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-20 5:29 ` Markus Armbruster
@ 2020-11-20 18:27 ` Eduardo Habkost
2020-11-23 7:51 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-20 18:27 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >>
> >> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> >
> >> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost <ehabkost@redhat.com>
> >> >> wrote:
> >> >> >
> >> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> >> > > literals without the referencing counting and memory allocation
> >> >> > > features provided by QObject.
> >> >> > >
> >> >> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> >> > > ---
> >> >> > > Changes v1 -> v2:
> >> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> >> > > use the new struct fields
> >> >> > > ---
> >> >> > > include/qapi/qmp/qnum.h | 40 +++++++++++++++++++--
> >> >> > > qobject/qnum.c | 78 ++++++++++++++++++++---------------------
> >> >> > > tests/check-qnum.c | 14 ++++----
> >> >> > > 3 files changed, 84 insertions(+), 48 deletions(-)
> >> >> > >
> >> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> >> > > index 55c27b1c24..62fbdfda68 100644
> >> >> > > --- a/include/qapi/qmp/qnum.h
> >> >> > > +++ b/include/qapi/qmp/qnum.h
> >> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> >> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> >> >> > > * convert under the hood.
> >> >> > > */
> >> >> > > -struct QNum {
> >> >> > > - struct QObjectBase_ base;
> >> >> > > +
> >> >> > > +/**
> >> >> > > + * struct QNumValue: the value of a QNum
> >> >> > > + *
> >> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> >> > > + */
> >> >> > > +typedef struct QNumValue {
> >> >> > > + /* private: */
> >>
> >> Do we care?
> >
> > Are you asking if we want to make them private, or if we want to
> > document them as private (assuming they are).
> >
> > The answer to the latter is yes ("private:" is an indication to
> > kernel-doc). The answer to the former seems to be "no", based on
> > your other comments.
> >
> > Or maybe `kind` should be public and `u` should be private?
>
> You're factoring out a part of struct QNum into new struct QNumValue.
> struct QNum is not private before the patch. I see no need to start
> making it or parts of it private now.
I don't want to change the rules, just to document the existing
implicit rules. If you say QNum.u was never private, I won't
argue.
>
> When the structure of a data type is to be kept away from its users, I
> prefer to keep it out of the public header, so the compiler enforces the
> encapsulation.
I prefer that too, except that it is impossible when users of the
API need the compiler to know the struct size.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-20 18:27 ` Eduardo Habkost
@ 2020-11-23 7:51 ` Markus Armbruster
2020-11-23 18:33 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-23 7:51 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé,
Marc-André Lureau, Markus Armbruster, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
[...]
>> When the structure of a data type is to be kept away from its users, I
>> prefer to keep it out of the public header, so the compiler enforces the
>> encapsulation.
>
> I prefer that too, except that it is impossible when users of the
> API need the compiler to know the struct size.
There are cases where the structure of a data type should be
encapsulated, yet its size must be made known for performance (avoid
dynamic memory allocation and pointer chasing).
Need for encapsulation correlates with complex algorithms and data
structures. The cost of dynamic allocation is often in the noise then.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-23 7:51 ` Markus Armbruster
@ 2020-11-23 18:33 ` Eduardo Habkost
2020-11-24 8:49 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-23 18:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> [...]
> >> When the structure of a data type is to be kept away from its users, I
> >> prefer to keep it out of the public header, so the compiler enforces the
> >> encapsulation.
> >
> > I prefer that too, except that it is impossible when users of the
> > API need the compiler to know the struct size.
>
> There are cases where the structure of a data type should be
> encapsulated, yet its size must be made known for performance (avoid
> dynamic memory allocation and pointer chasing).
>
> Need for encapsulation correlates with complex algorithms and data
> structures. The cost of dynamic allocation is often in the noise then.
I don't know what we are talking about anymore. None of this
applies to the QNum API, right?
QNum/QNumValue are not complex data structures, and the reason we
need the compiler to know the size of QNumValue is not related to
performance at all.
We might still want to discourage users of the QNum API from
accessing QNum.u/QNumValue.u directly. Documenting the field as
private is a very easy way to do it.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-23 18:33 ` Eduardo Habkost
@ 2020-11-24 8:49 ` Markus Armbruster
2020-11-24 14:41 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-24 8:49 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> [...]
>> >> When the structure of a data type is to be kept away from its users, I
>> >> prefer to keep it out of the public header, so the compiler enforces the
>> >> encapsulation.
>> >
>> > I prefer that too, except that it is impossible when users of the
>> > API need the compiler to know the struct size.
>>
>> There are cases where the structure of a data type should be
>> encapsulated, yet its size must be made known for performance (avoid
>> dynamic memory allocation and pointer chasing).
>>
>> Need for encapsulation correlates with complex algorithms and data
>> structures. The cost of dynamic allocation is often in the noise then.
>
> I don't know what we are talking about anymore. None of this
> applies to the QNum API, right?
>
> QNum/QNumValue are not complex data structures, and the reason we
> need the compiler to know the size of QNumValue is not related to
> performance at all.
We started with the question whether to make QNumValue's members
private. We digressed to the question when to make members private.
So back to the original question.
> We might still want to discourage users of the QNum API from
> accessing QNum.u/QNumValue.u directly. Documenting the field as
> private is a very easy way to do it.
It's a complete non-issue. QNum has been around for years, and we
haven't had any issues that could've been plausibly avoided by asking
people to refrain from accessing its members.
If there was an actual need to keep the members private, I'd move the
struct out of the header, so the compiler enforces privacy.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-24 8:49 ` Markus Armbruster
@ 2020-11-24 14:41 ` Eduardo Habkost
2020-11-24 15:20 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-24 14:41 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>
> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> >> [...]
> >> >> When the structure of a data type is to be kept away from its users, I
> >> >> prefer to keep it out of the public header, so the compiler enforces the
> >> >> encapsulation.
> >> >
> >> > I prefer that too, except that it is impossible when users of the
> >> > API need the compiler to know the struct size.
> >>
> >> There are cases where the structure of a data type should be
> >> encapsulated, yet its size must be made known for performance (avoid
> >> dynamic memory allocation and pointer chasing).
> >>
> >> Need for encapsulation correlates with complex algorithms and data
> >> structures. The cost of dynamic allocation is often in the noise then.
> >
> > I don't know what we are talking about anymore. None of this
> > applies to the QNum API, right?
> >
> > QNum/QNumValue are not complex data structures, and the reason we
> > need the compiler to know the size of QNumValue is not related to
> > performance at all.
>
> We started with the question whether to make QNumValue's members
> private. We digressed to the question when to make members private.
> So back to the original question.
>
> > We might still want to discourage users of the QNum API from
> > accessing QNum.u/QNumValue.u directly. Documenting the field as
> > private is a very easy way to do it.
>
> It's a complete non-issue. QNum has been around for years, and we
> haven't had any issues that could've been plausibly avoided by asking
> people to refrain from accessing its members.
>
> If there was an actual need to keep the members private, I'd move the
> struct out of the header, so the compiler enforces privacy.
Understood. There's still a question I'd like to answer, to
decide how the API documentation should look like:
Is QNum.u/QNumValue.u required to be part of the API
documentation?
If accessing that field directly is not necessary for using the
API, I don't think it should appear in the documentation (because
it would be just noise).
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-24 14:41 ` Eduardo Habkost
@ 2020-11-24 15:20 ` Markus Armbruster
2020-11-24 15:29 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-24 15:20 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >>
>> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> >> [...]
>> >> >> When the structure of a data type is to be kept away from its users, I
>> >> >> prefer to keep it out of the public header, so the compiler enforces the
>> >> >> encapsulation.
>> >> >
>> >> > I prefer that too, except that it is impossible when users of the
>> >> > API need the compiler to know the struct size.
>> >>
>> >> There are cases where the structure of a data type should be
>> >> encapsulated, yet its size must be made known for performance (avoid
>> >> dynamic memory allocation and pointer chasing).
>> >>
>> >> Need for encapsulation correlates with complex algorithms and data
>> >> structures. The cost of dynamic allocation is often in the noise then.
>> >
>> > I don't know what we are talking about anymore. None of this
>> > applies to the QNum API, right?
>> >
>> > QNum/QNumValue are not complex data structures, and the reason we
>> > need the compiler to know the size of QNumValue is not related to
>> > performance at all.
>>
>> We started with the question whether to make QNumValue's members
>> private. We digressed to the question when to make members private.
>> So back to the original question.
>>
>> > We might still want to discourage users of the QNum API from
>> > accessing QNum.u/QNumValue.u directly. Documenting the field as
>> > private is a very easy way to do it.
>>
>> It's a complete non-issue. QNum has been around for years, and we
>> haven't had any issues that could've been plausibly avoided by asking
>> people to refrain from accessing its members.
>>
>> If there was an actual need to keep the members private, I'd move the
>> struct out of the header, so the compiler enforces privacy.
>
> Understood. There's still a question I'd like to answer, to
> decide how the API documentation should look like:
>
> Is QNum.u/QNumValue.u required to be part of the API
> documentation?
>
> If accessing that field directly is not necessary for using the
> API, I don't think it should appear in the documentation (because
> it would be just noise).
The current patch's comment on QNumValue looks good to me.
Does this answer your question?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-24 15:20 ` Markus Armbruster
@ 2020-11-24 15:29 ` Eduardo Habkost
2020-11-25 6:40 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-24 15:29 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>
> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >>
> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> >> >> [...]
> >> >> >> When the structure of a data type is to be kept away from its users, I
> >> >> >> prefer to keep it out of the public header, so the compiler enforces the
> >> >> >> encapsulation.
> >> >> >
> >> >> > I prefer that too, except that it is impossible when users of the
> >> >> > API need the compiler to know the struct size.
> >> >>
> >> >> There are cases where the structure of a data type should be
> >> >> encapsulated, yet its size must be made known for performance (avoid
> >> >> dynamic memory allocation and pointer chasing).
> >> >>
> >> >> Need for encapsulation correlates with complex algorithms and data
> >> >> structures. The cost of dynamic allocation is often in the noise then.
> >> >
> >> > I don't know what we are talking about anymore. None of this
> >> > applies to the QNum API, right?
> >> >
> >> > QNum/QNumValue are not complex data structures, and the reason we
> >> > need the compiler to know the size of QNumValue is not related to
> >> > performance at all.
> >>
> >> We started with the question whether to make QNumValue's members
> >> private. We digressed to the question when to make members private.
> >> So back to the original question.
> >>
> >> > We might still want to discourage users of the QNum API from
> >> > accessing QNum.u/QNumValue.u directly. Documenting the field as
> >> > private is a very easy way to do it.
> >>
> >> It's a complete non-issue. QNum has been around for years, and we
> >> haven't had any issues that could've been plausibly avoided by asking
> >> people to refrain from accessing its members.
> >>
> >> If there was an actual need to keep the members private, I'd move the
> >> struct out of the header, so the compiler enforces privacy.
> >
> > Understood. There's still a question I'd like to answer, to
> > decide how the API documentation should look like:
> >
> > Is QNum.u/QNumValue.u required to be part of the API
> > documentation?
> >
> > If accessing that field directly is not necessary for using the
> > API, I don't think it should appear in the documentation (because
> > it would be just noise).
>
> The current patch's comment on QNumValue looks good to me.
>
> Does this answer your question?
The current patch (v3) doesn't address the question. It doesn't
include documentation for the field, but doesn't hide it.
kernel-doc will print a warning on that case.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-24 15:29 ` Eduardo Habkost
@ 2020-11-25 6:40 ` Markus Armbruster
2020-11-25 15:01 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-25 6:40 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >>
>> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> >>
>> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> >> >> [...]
>> >> >> >> When the structure of a data type is to be kept away from its users, I
>> >> >> >> prefer to keep it out of the public header, so the compiler enforces the
>> >> >> >> encapsulation.
>> >> >> >
>> >> >> > I prefer that too, except that it is impossible when users of the
>> >> >> > API need the compiler to know the struct size.
>> >> >>
>> >> >> There are cases where the structure of a data type should be
>> >> >> encapsulated, yet its size must be made known for performance (avoid
>> >> >> dynamic memory allocation and pointer chasing).
>> >> >>
>> >> >> Need for encapsulation correlates with complex algorithms and data
>> >> >> structures. The cost of dynamic allocation is often in the noise then.
>> >> >
>> >> > I don't know what we are talking about anymore. None of this
>> >> > applies to the QNum API, right?
>> >> >
>> >> > QNum/QNumValue are not complex data structures, and the reason we
>> >> > need the compiler to know the size of QNumValue is not related to
>> >> > performance at all.
>> >>
>> >> We started with the question whether to make QNumValue's members
>> >> private. We digressed to the question when to make members private.
>> >> So back to the original question.
>> >>
>> >> > We might still want to discourage users of the QNum API from
>> >> > accessing QNum.u/QNumValue.u directly. Documenting the field as
>> >> > private is a very easy way to do it.
>> >>
>> >> It's a complete non-issue. QNum has been around for years, and we
>> >> haven't had any issues that could've been plausibly avoided by asking
>> >> people to refrain from accessing its members.
>> >>
>> >> If there was an actual need to keep the members private, I'd move the
>> >> struct out of the header, so the compiler enforces privacy.
>> >
>> > Understood. There's still a question I'd like to answer, to
>> > decide how the API documentation should look like:
>> >
>> > Is QNum.u/QNumValue.u required to be part of the API
>> > documentation?
>> >
>> > If accessing that field directly is not necessary for using the
>> > API, I don't think it should appear in the documentation (because
>> > it would be just noise).
>>
>> The current patch's comment on QNumValue looks good to me.
>>
>> Does this answer your question?
>
> The current patch (v3) doesn't address the question. It doesn't
> include documentation for the field, but doesn't hide it.
> kernel-doc will print a warning on that case.
Do we care? How many such warnings exist before the patch? Does this
series add just this one, or more?
Use your judgement, then be ready to explain it :)
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
2020-11-25 6:40 ` Markus Armbruster
@ 2020-11-25 15:01 ` Eduardo Habkost
0 siblings, 0 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-25 15:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Wed, Nov 25, 2020 at 07:40:48AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>
> >> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >>
> >> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
> >> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >> >>
> >> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> >> >> >> [...]
> >> >> >> >> When the structure of a data type is to be kept away from its users, I
> >> >> >> >> prefer to keep it out of the public header, so the compiler enforces the
> >> >> >> >> encapsulation.
> >> >> >> >
> >> >> >> > I prefer that too, except that it is impossible when users of the
> >> >> >> > API need the compiler to know the struct size.
> >> >> >>
> >> >> >> There are cases where the structure of a data type should be
> >> >> >> encapsulated, yet its size must be made known for performance (avoid
> >> >> >> dynamic memory allocation and pointer chasing).
> >> >> >>
> >> >> >> Need for encapsulation correlates with complex algorithms and data
> >> >> >> structures. The cost of dynamic allocation is often in the noise then.
> >> >> >
> >> >> > I don't know what we are talking about anymore. None of this
> >> >> > applies to the QNum API, right?
> >> >> >
> >> >> > QNum/QNumValue are not complex data structures, and the reason we
> >> >> > need the compiler to know the size of QNumValue is not related to
> >> >> > performance at all.
> >> >>
> >> >> We started with the question whether to make QNumValue's members
> >> >> private. We digressed to the question when to make members private.
> >> >> So back to the original question.
> >> >>
> >> >> > We might still want to discourage users of the QNum API from
> >> >> > accessing QNum.u/QNumValue.u directly. Documenting the field as
> >> >> > private is a very easy way to do it.
> >> >>
> >> >> It's a complete non-issue. QNum has been around for years, and we
> >> >> haven't had any issues that could've been plausibly avoided by asking
> >> >> people to refrain from accessing its members.
> >> >>
> >> >> If there was an actual need to keep the members private, I'd move the
> >> >> struct out of the header, so the compiler enforces privacy.
> >> >
> >> > Understood. There's still a question I'd like to answer, to
> >> > decide how the API documentation should look like:
> >> >
> >> > Is QNum.u/QNumValue.u required to be part of the API
> >> > documentation?
> >> >
> >> > If accessing that field directly is not necessary for using the
> >> > API, I don't think it should appear in the documentation (because
> >> > it would be just noise).
> >>
> >> The current patch's comment on QNumValue looks good to me.
> >>
> >> Does this answer your question?
> >
> > The current patch (v3) doesn't address the question. It doesn't
> > include documentation for the field, but doesn't hide it.
> > kernel-doc will print a warning on that case.
>
> Do we care?
Yes. Peter will reject pull requests if it generates kernel-doc
warnings.
> How many such warnings exist before the patch?
Zero.
> Does this series add just this one, or more?
The current series (v3) doesn't add any, because I dropped the
patch that added QObject and QNum documentation to docs/devel. I
still want to resubmit that patch later, though.
>
> Use your judgement, then be ready to explain it :)
OK!
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
` (2 preceding siblings ...)
2020-11-16 22:41 ` [PATCH v2 3/8] qnum: QNumValue type for QNum value literals Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:42 ` Marc-André Lureau
2020-11-19 10:27 ` Markus Armbruster
2020-11-16 22:41 ` [PATCH v2 5/8] qlit: Support all types of QNums Eduardo Habkost
` (4 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
Extract the QNum value comparison logic to a function that takes
QNumValue* as argument.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/qapi/qmp/qnum.h | 1 +
qobject/qnum.c | 29 +++++++++++++++++++----------
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 62fbdfda68..0327ecd0f0 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
char *qnum_to_string(QNum *qn);
+bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y);
bool qnum_is_equal(const QObject *x, const QObject *y);
void qnum_destroy_obj(QObject *obj);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index f80d4efd76..6a0f948b16 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
}
/**
- * qnum_is_equal(): Test whether the two QNums are equal
- * @x: QNum object
- * @y: QNum object
+ * qnum_value_is_equal(): Test whether two QNumValues are equal
+ * @num_x: QNum value
+ * @num_y: QNum value
*
* Negative integers are never considered equal to unsigned integers,
* but positive integers in the range [0, INT64_MAX] are considered
@@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
*
* Doubles are never considered equal to integers.
*/
-bool qnum_is_equal(const QObject *x, const QObject *y)
+bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y)
{
- const QNum *qnum_x = qobject_to(QNum, x);
- const QNum *qnum_y = qobject_to(QNum, y);
- const QNumValue *num_x = &qnum_x->value;
- const QNumValue *num_y = &qnum_y->value;
-
switch (num_x->kind) {
case QNUM_I64:
switch (num_y->kind) {
@@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
case QNUM_U64:
switch (num_y->kind) {
case QNUM_I64:
- return qnum_is_equal(y, x);
+ return qnum_value_is_equal(num_y, num_x);
case QNUM_U64:
/* Comparison in native uint64_t type */
return num_x->u.u64 == num_y->u.u64;
@@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
abort();
}
+/**
+ * qnum_is_equal(): Test whether the two QNums are equal
+ * @x: QNum object
+ * @y: QNum object
+ *
+ * See qnum_value_is_equal() for details on the comparison rules.
+ */
+bool qnum_is_equal(const QObject *x, const QObject *y)
+{
+ const QNum *qnum_x = qobject_to(QNum, x);
+ const QNum *qnum_y = qobject_to(QNum, y);
+ return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
+}
+
/**
* qnum_destroy_obj(): Free all memory allocated by a QNum object
*
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-16 22:41 ` [PATCH v2 4/8] qnum: qnum_value_is_equal() function Eduardo Habkost
@ 2020-11-17 8:42 ` Marc-André Lureau
2020-11-17 15:49 ` Eduardo Habkost
2020-11-19 10:27 ` Markus Armbruster
1 sibling, 1 reply; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:42 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]
On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> Extract the QNum value comparison logic to a function that takes
> QNumValue* as argument.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> include/qapi/qmp/qnum.h | 1 +
> qobject/qnum.c | 29 +++++++++++++++++++----------
> 2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index 62fbdfda68..0327ecd0f0 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
>
> char *qnum_to_string(QNum *qn);
>
> +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y);
> bool qnum_is_equal(const QObject *x, const QObject *y);
> void qnum_destroy_obj(QObject *obj);
>
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index f80d4efd76..6a0f948b16 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> }
>
> /**
> - * qnum_is_equal(): Test whether the two QNums are equal
> - * @x: QNum object
> - * @y: QNum object
> + * qnum_value_is_equal(): Test whether two QNumValues are equal
> + * @num_x: QNum value
> + * @num_y: QNum value
>
val_x: a QNumValue ?
*
> * Negative integers are never considered equal to unsigned integers,
> * but positive integers in the range [0, INT64_MAX] are considered
> @@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
> *
> * Doubles are never considered equal to integers.
> */
> -bool qnum_is_equal(const QObject *x, const QObject *y)
> +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y)
> {
> - const QNum *qnum_x = qobject_to(QNum, x);
> - const QNum *qnum_y = qobject_to(QNum, y);
> - const QNumValue *num_x = &qnum_x->value;
> - const QNumValue *num_y = &qnum_y->value;
> -
> switch (num_x->kind) {
> case QNUM_I64:
> switch (num_y->kind) {
> @@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> case QNUM_U64:
> switch (num_y->kind) {
> case QNUM_I64:
> - return qnum_is_equal(y, x);
> + return qnum_value_is_equal(num_y, num_x);
> case QNUM_U64:
> /* Comparison in native uint64_t type */
> return num_x->u.u64 == num_y->u.u64;
> @@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> abort();
> }
>
> +/**
> + * qnum_is_equal(): Test whether the two QNums are equal
> + * @x: QNum object
> + * @y: QNum object
> + *
> + * See qnum_value_is_equal() for details on the comparison rules.
> + */
> +bool qnum_is_equal(const QObject *x, const QObject *y)
> +{
> + const QNum *qnum_x = qobject_to(QNum, x);
> + const QNum *qnum_y = qobject_to(QNum, y);
> + return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> +}
> +
> /**
> * qnum_destroy_obj(): Free all memory allocated by a QNum object
> *
> --
> 2.28.0
>
>
>
beside that,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 4227 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-17 8:42 ` Marc-André Lureau
@ 2020-11-17 15:49 ` Eduardo Habkost
2020-11-17 16:53 ` Marc-André Lureau
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-17 15:49 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
On Tue, Nov 17, 2020 at 12:42:47PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > Extract the QNum value comparison logic to a function that takes
> > QNumValue* as argument.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > include/qapi/qmp/qnum.h | 1 +
> > qobject/qnum.c | 29 +++++++++++++++++++----------
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > index 62fbdfda68..0327ecd0f0 100644
> > --- a/include/qapi/qmp/qnum.h
> > +++ b/include/qapi/qmp/qnum.h
> > @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
> >
> > char *qnum_to_string(QNum *qn);
> >
> > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y);
> > bool qnum_is_equal(const QObject *x, const QObject *y);
> > void qnum_destroy_obj(QObject *obj);
> >
> > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > index f80d4efd76..6a0f948b16 100644
> > --- a/qobject/qnum.c
> > +++ b/qobject/qnum.c
> > @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> > }
> >
> > /**
> > - * qnum_is_equal(): Test whether the two QNums are equal
> > - * @x: QNum object
> > - * @y: QNum object
> > + * qnum_value_is_equal(): Test whether two QNumValues are equal
> > + * @num_x: QNum value
> > + * @num_y: QNum value
> >
>
> val_x: a QNumValue ?
Do you mean:
@num_x: a QNumValue
?
I was not planning to rename the existing num_x/num_y variables.
>
> *
> > * Negative integers are never considered equal to unsigned integers,
> > * but positive integers in the range [0, INT64_MAX] are considered
> > @@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
> > *
> > * Doubles are never considered equal to integers.
> > */
> > -bool qnum_is_equal(const QObject *x, const QObject *y)
> > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y)
> > {
> > - const QNum *qnum_x = qobject_to(QNum, x);
> > - const QNum *qnum_y = qobject_to(QNum, y);
> > - const QNumValue *num_x = &qnum_x->value;
> > - const QNumValue *num_y = &qnum_y->value;
> > -
> > switch (num_x->kind) {
> > case QNUM_I64:
> > switch (num_y->kind) {
> > @@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> > case QNUM_U64:
> > switch (num_y->kind) {
> > case QNUM_I64:
> > - return qnum_is_equal(y, x);
> > + return qnum_value_is_equal(num_y, num_x);
> > case QNUM_U64:
> > /* Comparison in native uint64_t type */
> > return num_x->u.u64 == num_y->u.u64;
> > @@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> > abort();
> > }
> >
> > +/**
> > + * qnum_is_equal(): Test whether the two QNums are equal
> > + * @x: QNum object
> > + * @y: QNum object
> > + *
> > + * See qnum_value_is_equal() for details on the comparison rules.
> > + */
> > +bool qnum_is_equal(const QObject *x, const QObject *y)
> > +{
> > + const QNum *qnum_x = qobject_to(QNum, x);
> > + const QNum *qnum_y = qobject_to(QNum, y);
> > + return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > +}
> > +
> > /**
> > * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > *
> > --
> > 2.28.0
> >
> >
> >
> beside that,
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Thanks!
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-17 15:49 ` Eduardo Habkost
@ 2020-11-17 16:53 ` Marc-André Lureau
2020-11-17 17:21 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 16:53 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 4192 bytes --]
On Tue, Nov 17, 2020 at 7:49 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Nov 17, 2020 at 12:42:47PM +0400, Marc-André Lureau wrote:
> > On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> >
> > > Extract the QNum value comparison logic to a function that takes
> > > QNumValue* as argument.
> > >
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > include/qapi/qmp/qnum.h | 1 +
> > > qobject/qnum.c | 29 +++++++++++++++++++----------
> > > 2 files changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > > index 62fbdfda68..0327ecd0f0 100644
> > > --- a/include/qapi/qmp/qnum.h
> > > +++ b/include/qapi/qmp/qnum.h
> > > @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
> > >
> > > char *qnum_to_string(QNum *qn);
> > >
> > > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue
> *num_y);
> > > bool qnum_is_equal(const QObject *x, const QObject *y);
> > > void qnum_destroy_obj(QObject *obj);
> > >
> > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > > index f80d4efd76..6a0f948b16 100644
> > > --- a/qobject/qnum.c
> > > +++ b/qobject/qnum.c
> > > @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> > > }
> > >
> > > /**
> > > - * qnum_is_equal(): Test whether the two QNums are equal
> > > - * @x: QNum object
> > > - * @y: QNum object
> > > + * qnum_value_is_equal(): Test whether two QNumValues are equal
> > > + * @num_x: QNum value
> > > + * @num_y: QNum value
> > >
> >
> > val_x: a QNumValue ?
>
> Do you mean:
> @num_x: a QNumValue
>
?
>
> I was not planning to rename the existing num_x/num_y variables.
>
>
Not renaming because that would make some churn? But this is already quite
confusing, so it's better to use "val" for QNumVal and "num" for QNum I
guess.
If you don't want to rename it in the code, may I suggest doing it at the
declaration side? Not sure it's a better idea.
> >
> > *
> > > * Negative integers are never considered equal to unsigned integers,
> > > * but positive integers in the range [0, INT64_MAX] are considered
> > > @@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
> > > *
> > > * Doubles are never considered equal to integers.
> > > */
> > > -bool qnum_is_equal(const QObject *x, const QObject *y)
> > > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue
> *num_y)
> > > {
> > > - const QNum *qnum_x = qobject_to(QNum, x);
> > > - const QNum *qnum_y = qobject_to(QNum, y);
> > > - const QNumValue *num_x = &qnum_x->value;
> > > - const QNumValue *num_y = &qnum_y->value;
> > > -
> > > switch (num_x->kind) {
> > > case QNUM_I64:
> > > switch (num_y->kind) {
> > > @@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject
> *y)
> > > case QNUM_U64:
> > > switch (num_y->kind) {
> > > case QNUM_I64:
> > > - return qnum_is_equal(y, x);
> > > + return qnum_value_is_equal(num_y, num_x);
> > > case QNUM_U64:
> > > /* Comparison in native uint64_t type */
> > > return num_x->u.u64 == num_y->u.u64;
> > > @@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const
> QObject *y)
> > > abort();
> > > }
> > >
> > > +/**
> > > + * qnum_is_equal(): Test whether the two QNums are equal
> > > + * @x: QNum object
> > > + * @y: QNum object
> > > + *
> > > + * See qnum_value_is_equal() for details on the comparison rules.
> > > + */
> > > +bool qnum_is_equal(const QObject *x, const QObject *y)
> > > +{
> > > + const QNum *qnum_x = qobject_to(QNum, x);
> > > + const QNum *qnum_y = qobject_to(QNum, y);
> > > + return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > > +}
> > > +
> > > /**
> > > * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > > *
> > > --
> > > 2.28.0
> > >
> > >
> > >
> > beside that,
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Thanks!
>
> --
> Eduardo
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 6025 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-17 16:53 ` Marc-André Lureau
@ 2020-11-17 17:21 ` Eduardo Habkost
0 siblings, 0 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-17 17:21 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
On Tue, Nov 17, 2020 at 08:53:19PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 17, 2020 at 7:49 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Nov 17, 2020 at 12:42:47PM +0400, Marc-André Lureau wrote:
> > > On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost <ehabkost@redhat.com>
> > wrote:
> > >
> > > > Extract the QNum value comparison logic to a function that takes
> > > > QNumValue* as argument.
> > > >
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > include/qapi/qmp/qnum.h | 1 +
> > > > qobject/qnum.c | 29 +++++++++++++++++++----------
> > > > 2 files changed, 20 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > > > index 62fbdfda68..0327ecd0f0 100644
> > > > --- a/include/qapi/qmp/qnum.h
> > > > +++ b/include/qapi/qmp/qnum.h
> > > > @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
> > > >
> > > > char *qnum_to_string(QNum *qn);
> > > >
> > > > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue
> > *num_y);
> > > > bool qnum_is_equal(const QObject *x, const QObject *y);
> > > > void qnum_destroy_obj(QObject *obj);
> > > >
> > > > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > > > index f80d4efd76..6a0f948b16 100644
> > > > --- a/qobject/qnum.c
> > > > +++ b/qobject/qnum.c
> > > > @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> > > > }
> > > >
> > > > /**
> > > > - * qnum_is_equal(): Test whether the two QNums are equal
> > > > - * @x: QNum object
> > > > - * @y: QNum object
> > > > + * qnum_value_is_equal(): Test whether two QNumValues are equal
> > > > + * @num_x: QNum value
> > > > + * @num_y: QNum value
> > > >
> > >
> > > val_x: a QNumValue ?
> >
> > Do you mean:
> > @num_x: a QNumValue
> >
> ?
> >
> > I was not planning to rename the existing num_x/num_y variables.
> >
> >
> Not renaming because that would make some churn? But this is already quite
> confusing, so it's better to use "val" for QNumVal and "num" for QNum I
> guess.
>
> If you don't want to rename it in the code, may I suggest doing it at the
> declaration side? Not sure it's a better idea.
Yeah, I was not renaming them just to avoid churn.
However, I'm already replacing `qn` variables with `qv` at patch
3/8. Replacing num_x/num_y with val_x/val_y at qnum_is_equal()
(at patch 3/8 as well) wouldn't hurt too much.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-16 22:41 ` [PATCH v2 4/8] qnum: qnum_value_is_equal() function Eduardo Habkost
2020-11-17 8:42 ` Marc-André Lureau
@ 2020-11-19 10:27 ` Markus Armbruster
2020-11-19 18:24 ` Eduardo Habkost
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-19 10:27 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> Extract the QNum value comparison logic to a function that takes
> QNumValue* as argument.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> include/qapi/qmp/qnum.h | 1 +
> qobject/qnum.c | 29 +++++++++++++++++++----------
> 2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index 62fbdfda68..0327ecd0f0 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
>
> char *qnum_to_string(QNum *qn);
>
> +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y);
> bool qnum_is_equal(const QObject *x, const QObject *y);
> void qnum_destroy_obj(QObject *obj);
>
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index f80d4efd76..6a0f948b16 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> }
>
> /**
> - * qnum_is_equal(): Test whether the two QNums are equal
> - * @x: QNum object
> - * @y: QNum object
> + * qnum_value_is_equal(): Test whether two QNumValues are equal
> + * @num_x: QNum value
> + * @num_y: QNum value
> *
> * Negative integers are never considered equal to unsigned integers,
> * but positive integers in the range [0, INT64_MAX] are considered
> @@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
> *
> * Doubles are never considered equal to integers.
> */
> -bool qnum_is_equal(const QObject *x, const QObject *y)
> +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y)
> {
> - const QNum *qnum_x = qobject_to(QNum, x);
> - const QNum *qnum_y = qobject_to(QNum, y);
> - const QNumValue *num_x = &qnum_x->value;
> - const QNumValue *num_y = &qnum_y->value;
> -
> switch (num_x->kind) {
> case QNUM_I64:
> switch (num_y->kind) {
> @@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> case QNUM_U64:
> switch (num_y->kind) {
> case QNUM_I64:
> - return qnum_is_equal(y, x);
> + return qnum_value_is_equal(num_y, num_x);
> case QNUM_U64:
> /* Comparison in native uint64_t type */
> return num_x->u.u64 == num_y->u.u64;
> @@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> abort();
> }
>
> +/**
> + * qnum_is_equal(): Test whether the two QNums are equal
> + * @x: QNum object
> + * @y: QNum object
> + *
> + * See qnum_value_is_equal() for details on the comparison rules.
> + */
> +bool qnum_is_equal(const QObject *x, const QObject *y)
> +{
> + const QNum *qnum_x = qobject_to(QNum, x);
> + const QNum *qnum_y = qobject_to(QNum, y);
Humor me: blank line between declarations and statements, please.
> + return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> +}
> +
> /**
> * qnum_destroy_obj(): Free all memory allocated by a QNum object
> *
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-19 10:27 ` Markus Armbruster
@ 2020-11-19 18:24 ` Eduardo Habkost
2020-11-20 6:52 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 18:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
[...]
> > +bool qnum_is_equal(const QObject *x, const QObject *y)
> > +{
> > + const QNum *qnum_x = qobject_to(QNum, x);
> > + const QNum *qnum_y = qobject_to(QNum, y);
>
> Humor me: blank line between declarations and statements, please.
I can do it. But why do you prefer it that way?
>
> > + return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > +}
> > +
> > /**
> > * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > *
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-19 18:24 ` Eduardo Habkost
@ 2020-11-20 6:52 ` Markus Armbruster
2020-11-20 18:22 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-20 6:52 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
> [...]
>> > +bool qnum_is_equal(const QObject *x, const QObject *y)
>> > +{
>> > + const QNum *qnum_x = qobject_to(QNum, x);
>> > + const QNum *qnum_y = qobject_to(QNum, y);
>>
>> Humor me: blank line between declarations and statements, please.
>
> I can do it. But why do you prefer it that way?
Habit borne out of C lacking other visual cues to distinguish
declarations and statements.
Declaration or statement? Tell me quick, don't analyze!
mumble(*mutter)();
This "obviously" declares @mutter as pointer to function returning
mumble.
Except when @mumble isn't a typedef name, but a function taking one
argument and returning a function that takes no argument. Then it
passes *mutter to mumble(), and calls its return value.
The whole point of coding style is to help readers along. Two stylistic
conventions that can help here:
1. In a function call, no space between the expression denoting the
called function and the (parenthesized) argument list. Elsewhere,
space.
So, when the example above is indeed a declaration, write it as
mumble (*mutter)();
If it's function calls, write it as
mumble(*mutter)();
2. Separate declarations from statements with a blank line. Do not mix
them.
[...]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-20 6:52 ` Markus Armbruster
@ 2020-11-20 18:22 ` Eduardo Habkost
2020-11-23 8:17 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-20 18:22 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
On Fri, Nov 20, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
> > [...]
> >> > +bool qnum_is_equal(const QObject *x, const QObject *y)
> >> > +{
> >> > + const QNum *qnum_x = qobject_to(QNum, x);
> >> > + const QNum *qnum_y = qobject_to(QNum, y);
> >>
> >> Humor me: blank line between declarations and statements, please.
> >
> > I can do it. But why do you prefer it that way?
>
> Habit borne out of C lacking other visual cues to distinguish
> declarations and statements.
Why is the distinction important, when many variable declarations
also include initializer expressions that can be as complex as
other statements?
(The qobject_to() calls above are an example).
>
> Declaration or statement? Tell me quick, don't analyze!
>
> mumble(*mutter)();
>
> This "obviously" declares @mutter as pointer to function returning
> mumble.
>
> Except when @mumble isn't a typedef name, but a function taking one
> argument and returning a function that takes no argument. Then it
> passes *mutter to mumble(), and calls its return value.
>
> The whole point of coding style is to help readers along. Two stylistic
> conventions that can help here:
>
> 1. In a function call, no space between the expression denoting the
> called function and the (parenthesized) argument list. Elsewhere,
> space.
>
> So, when the example above is indeed a declaration, write it as
>
> mumble (*mutter)();
>
> If it's function calls, write it as
>
> mumble(*mutter)();
This makes lots of sense. Starting with a word followed by space
is what makes declarations visually distinguishable.
>
> 2. Separate declarations from statements with a blank line. Do not mix
> them.
I'm not sure about this one, and I'm actually glad it is not part
of CODING_STYLE. :)
(I'll still follow your advice as maintainer of that piece of
code, of course)
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
2020-11-20 18:22 ` Eduardo Habkost
@ 2020-11-23 8:17 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2020-11-23 8:17 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Nov 20, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>> > On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
>> > [...]
>> >> > +bool qnum_is_equal(const QObject *x, const QObject *y)
>> >> > +{
>> >> > + const QNum *qnum_x = qobject_to(QNum, x);
>> >> > + const QNum *qnum_y = qobject_to(QNum, y);
>> >>
>> >> Humor me: blank line between declarations and statements, please.
>> >
>> > I can do it. But why do you prefer it that way?
>>
>> Habit borne out of C lacking other visual cues to distinguish
>> declarations and statements.
>
> Why is the distinction important, when many variable declarations
> also include initializer expressions that can be as complex as
> other statements?
>
> (The qobject_to() calls above are an example).
We read left to right, and we're not good at backtracking. The earlier
I know I'm reading a declaration, the better.
>> Declaration or statement? Tell me quick, don't analyze!
>>
>> mumble(*mutter)();
>>
>> This "obviously" declares @mutter as pointer to function returning
>> mumble.
>>
>> Except when @mumble isn't a typedef name, but a function taking one
>> argument and returning a function that takes no argument. Then it
>> passes *mutter to mumble(), and calls its return value.
>>
>> The whole point of coding style is to help readers along. Two stylistic
>> conventions that can help here:
>>
>> 1. In a function call, no space between the expression denoting the
>> called function and the (parenthesized) argument list. Elsewhere,
>> space.
>>
>> So, when the example above is indeed a declaration, write it as
>>
>> mumble (*mutter)();
>>
>> If it's function calls, write it as
>>
>> mumble(*mutter)();
>
> This makes lots of sense. Starting with a word followed by space
> is what makes declarations visually distinguishable.
Declarations need not match that pattern. Also, it's a rather subtle
cue.
>> 2. Separate declarations from statements with a blank line. Do not mix
>> them.
>
> I'm not sure about this one, and I'm actually glad it is not part
> of CODING_STYLE. :)
That's why I ask to "humor me" :)
For what it's worth, the convention is common enough to be supported by
the traditional BSD indent program.
> (I'll still follow your advice as maintainer of that piece of
> code, of course)
Thanks!
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 5/8] qlit: Support all types of QNums
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
` (3 preceding siblings ...)
2020-11-16 22:41 ` [PATCH v2 4/8] qnum: qnum_value_is_equal() function Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:52 ` Marc-André Lureau
2020-11-16 22:41 ` [PATCH v2 6/8] qlit: qlit_type() function Eduardo Habkost
` (3 subsequent siblings)
8 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
Use QNumValue to represent QNums, so we can also support uint64_t
and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
macros for each case.
The QLIT_QNUM() macro is being kept for compatibility with
existing code, but becomes just a wrapper for QLIT_QNUM_INT().
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Coding style fix at qlit_equal_qobject()
---
include/qapi/qmp/qlit.h | 11 +++++--
qobject/qlit.c | 5 +--
tests/check-qjson.c | 72 ++++++++++++++++++++++++++++++++++++++---
3 files changed, 79 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index c0676d5daf..f9e356d31e 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -15,6 +15,7 @@
#define QLIT_H
#include "qobject.h"
+#include "qnum.h"
typedef struct QLitDictEntry QLitDictEntry;
typedef struct QLitObject QLitObject;
@@ -23,7 +24,7 @@ struct QLitObject {
QType type;
union {
bool qbool;
- int64_t qnum;
+ QNumValue qnum;
const char *qstr;
QLitDictEntry *qdict;
QLitObject *qlist;
@@ -39,8 +40,14 @@ struct QLitDictEntry {
{ .type = QTYPE_QNULL }
#define QLIT_QBOOL(val) \
{ .type = QTYPE_QBOOL, .value.qbool = (val) }
+#define QLIT_QNUM_INT(val) \
+ { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
+#define QLIT_QNUM_UINT(val) \
+ { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_UINT(val) }
+#define QLIT_QNUM_DOUBLE(val) \
+ { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_DOUBLE(val) }
#define QLIT_QNUM(val) \
- { .type = QTYPE_QNUM, .value.qnum = (val) }
+ QLIT_QNUM_INT(val)
#define QLIT_QSTR(val) \
{ .type = QTYPE_QSTRING, .value.qstr = (val) }
#define QLIT_QDICT(val) \
diff --git a/qobject/qlit.c b/qobject/qlit.c
index be8332136c..b23cdc4532 100644
--- a/qobject/qlit.c
+++ b/qobject/qlit.c
@@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject *rhs)
case QTYPE_QBOOL:
return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
case QTYPE_QNUM:
- return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
+ return qnum_value_is_equal(&lhs->value.qnum,
+ qnum_get_value(qobject_to(QNum, rhs)));
case QTYPE_QSTRING:
return (strcmp(lhs->value.qstr,
qstring_get_str(qobject_to(QString, rhs))) == 0);
@@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
case QTYPE_QNULL:
return QOBJECT(qnull());
case QTYPE_QNUM:
- return QOBJECT(qnum_from_int(qlit->value.qnum));
+ return QOBJECT(qnum_from_value(qlit->value.qnum));
case QTYPE_QSTRING:
return QOBJECT(qstring_from_str(qlit->value.qstr));
case QTYPE_QDICT: {
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 07a773e653..711030cffd 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -796,20 +796,23 @@ static void simple_number(void)
int i;
struct {
const char *encoded;
+ QLitObject qlit;
int64_t decoded;
int skip;
} test_cases[] = {
- { "0", 0 },
- { "1234", 1234 },
- { "1", 1 },
- { "-32", -32 },
- { "-0", 0, .skip = 1 },
+ { "0", QLIT_QNUM(0), 0, },
+ { "1234", QLIT_QNUM(1234), 1234, },
+ { "1", QLIT_QNUM(1), 1, },
+ { "-32", QLIT_QNUM(-32), -32, },
+ { "-0", QLIT_QNUM(0), 0, .skip = 1 },
{ },
};
for (i = 0; test_cases[i].encoded; i++) {
QNum *qnum;
int64_t val;
+ QNum *qlit_num;
+ int64_t qlit_val;
qnum = qobject_to(QNum,
qobject_from_json(test_cases[i].encoded,
@@ -817,6 +820,7 @@ static void simple_number(void)
g_assert(qnum);
g_assert(qnum_get_try_int(qnum, &val));
g_assert_cmpint(val, ==, test_cases[i].decoded);
+
if (test_cases[i].skip == 0) {
QString *str;
@@ -826,9 +830,66 @@ static void simple_number(void)
}
qobject_unref(qnum);
+
+ qlit_num = qobject_to(QNum,
+ qobject_from_qlit(&test_cases[i].qlit));
+ g_assert(qlit_num);
+ g_assert(qnum_get_try_int(qlit_num, &qlit_val));
+ g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
+
+ qobject_unref(qlit_num);
}
}
+static void qlit_large_number(void)
+{
+ QLitObject maxu64 = QLIT_QNUM_UINT(UINT64_MAX);
+ QLitObject maxi64 = QLIT_QNUM(INT64_MAX);
+ QLitObject mini64 = QLIT_QNUM(INT64_MIN);
+ QLitObject gtu64 = QLIT_QNUM_DOUBLE(18446744073709552e3);
+ QLitObject lti64 = QLIT_QNUM_DOUBLE(-92233720368547758e2);
+ QNum *qnum;
+ uint64_t val;
+ int64_t ival;
+
+ qnum = qobject_to(QNum, qobject_from_qlit(&maxu64));
+ g_assert(qnum);
+ g_assert_cmpuint(qnum_get_uint(qnum), ==, UINT64_MAX);
+ g_assert(!qnum_get_try_int(qnum, &ival));
+
+ qobject_unref(qnum);
+
+ qnum = qobject_to(QNum, qobject_from_qlit(&maxi64));
+ g_assert(qnum);
+ g_assert_cmpuint(qnum_get_uint(qnum), ==, INT64_MAX);
+ g_assert_cmpint(qnum_get_int(qnum), ==, INT64_MAX);
+
+ qobject_unref(qnum);
+
+ qnum = qobject_to(QNum, qobject_from_qlit(&mini64));
+ g_assert(qnum);
+ g_assert(!qnum_get_try_uint(qnum, &val));
+ g_assert_cmpuint(qnum_get_int(qnum), ==, INT64_MIN);
+
+ qobject_unref(qnum);
+
+ qnum = qobject_to(QNum, qobject_from_qlit(>u64));
+ g_assert(qnum);
+ g_assert_cmpfloat(qnum_get_double(qnum), ==, 18446744073709552e3);
+ g_assert(!qnum_get_try_uint(qnum, &val));
+ g_assert(!qnum_get_try_int(qnum, &ival));
+
+ qobject_unref(qnum);
+
+ qnum = qobject_to(QNum, qobject_from_qlit(<i64));
+ g_assert(qnum);
+ g_assert_cmpfloat(qnum_get_double(qnum), ==, -92233720368547758e2);
+ g_assert(!qnum_get_try_uint(qnum, &val));
+ g_assert(!qnum_get_try_int(qnum, &ival));
+
+ qobject_unref(qnum);
+}
+
static void large_number(void)
{
const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
@@ -1472,6 +1533,7 @@ int main(int argc, char **argv)
g_test_add_func("/literals/string/utf8", utf8_string);
g_test_add_func("/literals/number/simple", simple_number);
+ g_test_add_func("/literals/number/qlit_large", qlit_large_number);
g_test_add_func("/literals/number/large", large_number);
g_test_add_func("/literals/number/float", float_number);
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 5/8] qlit: Support all types of QNums
2020-11-16 22:41 ` [PATCH v2 5/8] qlit: Support all types of QNums Eduardo Habkost
@ 2020-11-17 8:52 ` Marc-André Lureau
2020-11-19 10:39 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:52 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 7355 bytes --]
On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> Use QNumValue to represent QNums, so we can also support uint64_t
> and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
> macros for each case.
>
> The QLIT_QNUM() macro is being kept for compatibility with
> existing code, but becomes just a wrapper for QLIT_QNUM_INT().
>
I am not sure it's worth to keep. (furthermore, it's only used in tests
afaics)
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
> Changes v1 -> v2:
> * Coding style fix at qlit_equal_qobject()
> ---
> include/qapi/qmp/qlit.h | 11 +++++--
> qobject/qlit.c | 5 +--
> tests/check-qjson.c | 72 ++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 79 insertions(+), 9 deletions(-)
>
> diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
> index c0676d5daf..f9e356d31e 100644
> --- a/include/qapi/qmp/qlit.h
> +++ b/include/qapi/qmp/qlit.h
> @@ -15,6 +15,7 @@
> #define QLIT_H
>
> #include "qobject.h"
> +#include "qnum.h"
>
> typedef struct QLitDictEntry QLitDictEntry;
> typedef struct QLitObject QLitObject;
> @@ -23,7 +24,7 @@ struct QLitObject {
> QType type;
> union {
> bool qbool;
> - int64_t qnum;
> + QNumValue qnum;
> const char *qstr;
> QLitDictEntry *qdict;
> QLitObject *qlist;
> @@ -39,8 +40,14 @@ struct QLitDictEntry {
> { .type = QTYPE_QNULL }
> #define QLIT_QBOOL(val) \
> { .type = QTYPE_QBOOL, .value.qbool = (val) }
> +#define QLIT_QNUM_INT(val) \
> + { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
> +#define QLIT_QNUM_UINT(val) \
> + { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_UINT(val) }
> +#define QLIT_QNUM_DOUBLE(val) \
> + { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_DOUBLE(val) }
> #define QLIT_QNUM(val) \
> - { .type = QTYPE_QNUM, .value.qnum = (val) }
> + QLIT_QNUM_INT(val)
> #define QLIT_QSTR(val) \
> { .type = QTYPE_QSTRING, .value.qstr = (val) }
> #define QLIT_QDICT(val) \
> diff --git a/qobject/qlit.c b/qobject/qlit.c
> index be8332136c..b23cdc4532 100644
> --- a/qobject/qlit.c
> +++ b/qobject/qlit.c
> @@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> QObject *rhs)
> case QTYPE_QBOOL:
> return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
> case QTYPE_QNUM:
> - return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
> + return qnum_value_is_equal(&lhs->value.qnum,
> + qnum_get_value(qobject_to(QNum, rhs)));
> case QTYPE_QSTRING:
> return (strcmp(lhs->value.qstr,
> qstring_get_str(qobject_to(QString, rhs))) == 0);
> @@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
> case QTYPE_QNULL:
> return QOBJECT(qnull());
> case QTYPE_QNUM:
> - return QOBJECT(qnum_from_int(qlit->value.qnum));
> + return QOBJECT(qnum_from_value(qlit->value.qnum));
> case QTYPE_QSTRING:
> return QOBJECT(qstring_from_str(qlit->value.qstr));
> case QTYPE_QDICT: {
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index 07a773e653..711030cffd 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -796,20 +796,23 @@ static void simple_number(void)
> int i;
> struct {
> const char *encoded;
> + QLitObject qlit;
> int64_t decoded;
> int skip;
> } test_cases[] = {
> - { "0", 0 },
> - { "1234", 1234 },
> - { "1", 1 },
> - { "-32", -32 },
> - { "-0", 0, .skip = 1 },
> + { "0", QLIT_QNUM(0), 0, },
> + { "1234", QLIT_QNUM(1234), 1234, },
> + { "1", QLIT_QNUM(1), 1, },
> + { "-32", QLIT_QNUM(-32), -32, },
> + { "-0", QLIT_QNUM(0), 0, .skip = 1 },
> { },
> };
>
> for (i = 0; test_cases[i].encoded; i++) {
> QNum *qnum;
> int64_t val;
> + QNum *qlit_num;
> + int64_t qlit_val;
>
> qnum = qobject_to(QNum,
> qobject_from_json(test_cases[i].encoded,
> @@ -817,6 +820,7 @@ static void simple_number(void)
> g_assert(qnum);
> g_assert(qnum_get_try_int(qnum, &val));
> g_assert_cmpint(val, ==, test_cases[i].decoded);
> +
> if (test_cases[i].skip == 0) {
> QString *str;
>
> @@ -826,9 +830,66 @@ static void simple_number(void)
> }
>
> qobject_unref(qnum);
> +
> + qlit_num = qobject_to(QNum,
> + qobject_from_qlit(&test_cases[i].qlit));
> + g_assert(qlit_num);
> + g_assert(qnum_get_try_int(qlit_num, &qlit_val));
> + g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
> +
> + qobject_unref(qlit_num);
> }
> }
>
> +static void qlit_large_number(void)
> +{
> + QLitObject maxu64 = QLIT_QNUM_UINT(UINT64_MAX);
> + QLitObject maxi64 = QLIT_QNUM(INT64_MAX);
> + QLitObject mini64 = QLIT_QNUM(INT64_MIN);
> + QLitObject gtu64 = QLIT_QNUM_DOUBLE(18446744073709552e3);
> + QLitObject lti64 = QLIT_QNUM_DOUBLE(-92233720368547758e2);
> + QNum *qnum;
> + uint64_t val;
> + int64_t ival;
> +
> + qnum = qobject_to(QNum, qobject_from_qlit(&maxu64));
> + g_assert(qnum);
> + g_assert_cmpuint(qnum_get_uint(qnum), ==, UINT64_MAX);
> + g_assert(!qnum_get_try_int(qnum, &ival));
> +
> + qobject_unref(qnum);
> +
> + qnum = qobject_to(QNum, qobject_from_qlit(&maxi64));
> + g_assert(qnum);
> + g_assert_cmpuint(qnum_get_uint(qnum), ==, INT64_MAX);
> + g_assert_cmpint(qnum_get_int(qnum), ==, INT64_MAX);
> +
> + qobject_unref(qnum);
> +
> + qnum = qobject_to(QNum, qobject_from_qlit(&mini64));
> + g_assert(qnum);
> + g_assert(!qnum_get_try_uint(qnum, &val));
> + g_assert_cmpuint(qnum_get_int(qnum), ==, INT64_MIN);
> +
> + qobject_unref(qnum);
> +
> + qnum = qobject_to(QNum, qobject_from_qlit(>u64));
> + g_assert(qnum);
> + g_assert_cmpfloat(qnum_get_double(qnum), ==, 18446744073709552e3);
> + g_assert(!qnum_get_try_uint(qnum, &val));
> + g_assert(!qnum_get_try_int(qnum, &ival));
> +
> + qobject_unref(qnum);
> +
> + qnum = qobject_to(QNum, qobject_from_qlit(<i64));
> + g_assert(qnum);
> + g_assert_cmpfloat(qnum_get_double(qnum), ==, -92233720368547758e2);
> + g_assert(!qnum_get_try_uint(qnum, &val));
> + g_assert(!qnum_get_try_int(qnum, &ival));
> +
> + qobject_unref(qnum);
> +}
> +
> static void large_number(void)
> {
> const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
> @@ -1472,6 +1533,7 @@ int main(int argc, char **argv)
> g_test_add_func("/literals/string/utf8", utf8_string);
>
> g_test_add_func("/literals/number/simple", simple_number);
> + g_test_add_func("/literals/number/qlit_large", qlit_large_number);
> g_test_add_func("/literals/number/large", large_number);
> g_test_add_func("/literals/number/float", float_number);
>
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 9338 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 5/8] qlit: Support all types of QNums
2020-11-17 8:52 ` Marc-André Lureau
@ 2020-11-19 10:39 ` Markus Armbruster
2020-11-19 18:56 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-19 10:39 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, QEMU
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> Use QNumValue to represent QNums, so we can also support uint64_t
>> and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
>> macros for each case.
>>
>> The QLIT_QNUM() macro is being kept for compatibility with
>> existing code, but becomes just a wrapper for QLIT_QNUM_INT().
>>
>
> I am not sure it's worth to keep. (furthermore, it's only used in tests
> afaics)
Seconded.
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
>> Changes v1 -> v2:
>> * Coding style fix at qlit_equal_qobject()
>> ---
>> include/qapi/qmp/qlit.h | 11 +++++--
>> qobject/qlit.c | 5 +--
>> tests/check-qjson.c | 72 ++++++++++++++++++++++++++++++++++++++---
>> 3 files changed, 79 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
>> index c0676d5daf..f9e356d31e 100644
>> --- a/include/qapi/qmp/qlit.h
>> +++ b/include/qapi/qmp/qlit.h
>> @@ -15,6 +15,7 @@
>> #define QLIT_H
>>
>> #include "qobject.h"
>> +#include "qnum.h"
>>
>> typedef struct QLitDictEntry QLitDictEntry;
>> typedef struct QLitObject QLitObject;
>> @@ -23,7 +24,7 @@ struct QLitObject {
>> QType type;
>> union {
>> bool qbool;
>> - int64_t qnum;
>> + QNumValue qnum;
>> const char *qstr;
>> QLitDictEntry *qdict;
>> QLitObject *qlist;
>> @@ -39,8 +40,14 @@ struct QLitDictEntry {
>> { .type = QTYPE_QNULL }
>> #define QLIT_QBOOL(val) \
>> { .type = QTYPE_QBOOL, .value.qbool = (val) }
>> +#define QLIT_QNUM_INT(val) \
>> + { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
>> +#define QLIT_QNUM_UINT(val) \
>> + { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_UINT(val) }
>> +#define QLIT_QNUM_DOUBLE(val) \
>> + { .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_DOUBLE(val) }
>> #define QLIT_QNUM(val) \
>> - { .type = QTYPE_QNUM, .value.qnum = (val) }
>> + QLIT_QNUM_INT(val)
>> #define QLIT_QSTR(val) \
>> { .type = QTYPE_QSTRING, .value.qstr = (val) }
>> #define QLIT_QDICT(val) \
>> diff --git a/qobject/qlit.c b/qobject/qlit.c
>> index be8332136c..b23cdc4532 100644
>> --- a/qobject/qlit.c
>> +++ b/qobject/qlit.c
>> @@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
>> QObject *rhs)
>> case QTYPE_QBOOL:
>> return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
>> case QTYPE_QNUM:
>> - return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
>> + return qnum_value_is_equal(&lhs->value.qnum,
>> + qnum_get_value(qobject_to(QNum, rhs)));
Before the patch, we crash when @rhs can't be represented as int64_t.
Afterwards, we return false (I think).
Please note this in the commit message. A separate fix preceding this
patch would be even better, but may not be worth the trouble. Up to
you.
>> case QTYPE_QSTRING:
>> return (strcmp(lhs->value.qstr,
>> qstring_get_str(qobject_to(QString, rhs))) == 0);
>> @@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
>> case QTYPE_QNULL:
>> return QOBJECT(qnull());
>> case QTYPE_QNUM:
>> - return QOBJECT(qnum_from_int(qlit->value.qnum));
>> + return QOBJECT(qnum_from_value(qlit->value.qnum));
>> case QTYPE_QSTRING:
>> return QOBJECT(qstring_from_str(qlit->value.qstr));
>> case QTYPE_QDICT: {
>> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>> index 07a773e653..711030cffd 100644
>> --- a/tests/check-qjson.c
>> +++ b/tests/check-qjson.c
>> @@ -796,20 +796,23 @@ static void simple_number(void)
>> int i;
>> struct {
>> const char *encoded;
>> + QLitObject qlit;
>> int64_t decoded;
>> int skip;
>> } test_cases[] = {
>> - { "0", 0 },
>> - { "1234", 1234 },
>> - { "1", 1 },
>> - { "-32", -32 },
>> - { "-0", 0, .skip = 1 },
>> + { "0", QLIT_QNUM(0), 0, },
>> + { "1234", QLIT_QNUM(1234), 1234, },
>> + { "1", QLIT_QNUM(1), 1, },
>> + { "-32", QLIT_QNUM(-32), -32, },
>> + { "-0", QLIT_QNUM(0), 0, .skip = 1 },
Note .qlit is always QLIT_QNUM(.decoded). Would doing without .qlit
result in a simpler patch?
>> { },
>> };
>>
>> for (i = 0; test_cases[i].encoded; i++) {
>> QNum *qnum;
>> int64_t val;
>> + QNum *qlit_num;
>> + int64_t qlit_val;
>>
>> qnum = qobject_to(QNum,
>> qobject_from_json(test_cases[i].encoded,
>> @@ -817,6 +820,7 @@ static void simple_number(void)
>> g_assert(qnum);
>> g_assert(qnum_get_try_int(qnum, &val));
>> g_assert_cmpint(val, ==, test_cases[i].decoded);
>> +
>> if (test_cases[i].skip == 0) {
>> QString *str;
>>
>> @@ -826,9 +830,66 @@ static void simple_number(void)
>> }
>>
>> qobject_unref(qnum);
>> +
>> + qlit_num = qobject_to(QNum,
>> + qobject_from_qlit(&test_cases[i].qlit));
>> + g_assert(qlit_num);
>> + g_assert(qnum_get_try_int(qlit_num, &qlit_val));
>> + g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
>> +
>> + qobject_unref(qlit_num);
>> }
>> }
>>
>> +static void qlit_large_number(void)
>> +{
>> + QLitObject maxu64 = QLIT_QNUM_UINT(UINT64_MAX);
>> + QLitObject maxi64 = QLIT_QNUM(INT64_MAX);
>> + QLitObject mini64 = QLIT_QNUM(INT64_MIN);
>> + QLitObject gtu64 = QLIT_QNUM_DOUBLE(18446744073709552e3);
>> + QLitObject lti64 = QLIT_QNUM_DOUBLE(-92233720368547758e2);
>> + QNum *qnum;
>> + uint64_t val;
>> + int64_t ival;
>> +
>> + qnum = qobject_to(QNum, qobject_from_qlit(&maxu64));
>> + g_assert(qnum);
>> + g_assert_cmpuint(qnum_get_uint(qnum), ==, UINT64_MAX);
>> + g_assert(!qnum_get_try_int(qnum, &ival));
>> +
>> + qobject_unref(qnum);
>> +
>> + qnum = qobject_to(QNum, qobject_from_qlit(&maxi64));
>> + g_assert(qnum);
>> + g_assert_cmpuint(qnum_get_uint(qnum), ==, INT64_MAX);
>> + g_assert_cmpint(qnum_get_int(qnum), ==, INT64_MAX);
>> +
>> + qobject_unref(qnum);
>> +
>> + qnum = qobject_to(QNum, qobject_from_qlit(&mini64));
>> + g_assert(qnum);
>> + g_assert(!qnum_get_try_uint(qnum, &val));
>> + g_assert_cmpuint(qnum_get_int(qnum), ==, INT64_MIN);
>> +
>> + qobject_unref(qnum);
>> +
>> + qnum = qobject_to(QNum, qobject_from_qlit(>u64));
>> + g_assert(qnum);
>> + g_assert_cmpfloat(qnum_get_double(qnum), ==, 18446744073709552e3);
>> + g_assert(!qnum_get_try_uint(qnum, &val));
>> + g_assert(!qnum_get_try_int(qnum, &ival));
>> +
>> + qobject_unref(qnum);
>> +
>> + qnum = qobject_to(QNum, qobject_from_qlit(<i64));
>> + g_assert(qnum);
>> + g_assert_cmpfloat(qnum_get_double(qnum), ==, -92233720368547758e2);
>> + g_assert(!qnum_get_try_uint(qnum, &val));
>> + g_assert(!qnum_get_try_int(qnum, &ival));
>> +
>> + qobject_unref(qnum);
>> +}
>> +
>> static void large_number(void)
>> {
>> const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
>> @@ -1472,6 +1533,7 @@ int main(int argc, char **argv)
>> g_test_add_func("/literals/string/utf8", utf8_string);
>>
>> g_test_add_func("/literals/number/simple", simple_number);
>> + g_test_add_func("/literals/number/qlit_large", qlit_large_number);
>> g_test_add_func("/literals/number/large", large_number);
>> g_test_add_func("/literals/number/float", float_number);
>>
>> --
>> 2.28.0
>>
>>
>>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 5/8] qlit: Support all types of QNums
2020-11-19 10:39 ` Markus Armbruster
@ 2020-11-19 18:56 ` Eduardo Habkost
2020-11-20 6:55 ` Markus Armbruster
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 18:56 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
On Thu, Nov 19, 2020 at 11:39:14AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> Use QNumValue to represent QNums, so we can also support uint64_t
> >> and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
> >> macros for each case.
> >>
> >> The QLIT_QNUM() macro is being kept for compatibility with
> >> existing code, but becomes just a wrapper for QLIT_QNUM_INT().
> >>
> >
> > I am not sure it's worth to keep. (furthermore, it's only used in tests
> > afaics)
>
> Seconded.
Understood, I will remove it. I thought the QAPI code generator
was using it.
[...]
> >> diff --git a/qobject/qlit.c b/qobject/qlit.c
> >> index be8332136c..b23cdc4532 100644
> >> --- a/qobject/qlit.c
> >> +++ b/qobject/qlit.c
> >> @@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> >> QObject *rhs)
> >> case QTYPE_QBOOL:
> >> return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
> >> case QTYPE_QNUM:
> >> - return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
> >> + return qnum_value_is_equal(&lhs->value.qnum,
> >> + qnum_get_value(qobject_to(QNum, rhs)));
>
> Before the patch, we crash when @rhs can't be represented as int64_t.
I thought it was expected behavior? QLit never supported
QNUM_U64 or QNUM_DOUBLE as input.
>
> Afterwards, we return false (I think).
>
> Please note this in the commit message. A separate fix preceding this
> patch would be even better, but may not be worth the trouble. Up to
> you.
The fix would be 3 or 4 extra lines of code that would be
immediately deleted. I'll just mention it as a side effect of
the new feature.
>
> >> case QTYPE_QSTRING:
> >> return (strcmp(lhs->value.qstr,
> >> qstring_get_str(qobject_to(QString, rhs))) == 0);
> >> @@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
> >> case QTYPE_QNULL:
> >> return QOBJECT(qnull());
> >> case QTYPE_QNUM:
> >> - return QOBJECT(qnum_from_int(qlit->value.qnum));
> >> + return QOBJECT(qnum_from_value(qlit->value.qnum));
> >> case QTYPE_QSTRING:
> >> return QOBJECT(qstring_from_str(qlit->value.qstr));
> >> case QTYPE_QDICT: {
> >> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> >> index 07a773e653..711030cffd 100644
> >> --- a/tests/check-qjson.c
> >> +++ b/tests/check-qjson.c
> >> @@ -796,20 +796,23 @@ static void simple_number(void)
> >> int i;
> >> struct {
> >> const char *encoded;
> >> + QLitObject qlit;
> >> int64_t decoded;
> >> int skip;
> >> } test_cases[] = {
> >> - { "0", 0 },
> >> - { "1234", 1234 },
> >> - { "1", 1 },
> >> - { "-32", -32 },
> >> - { "-0", 0, .skip = 1 },
> >> + { "0", QLIT_QNUM(0), 0, },
> >> + { "1234", QLIT_QNUM(1234), 1234, },
> >> + { "1", QLIT_QNUM(1), 1, },
> >> + { "-32", QLIT_QNUM(-32), -32, },
> >> + { "-0", QLIT_QNUM(0), 0, .skip = 1 },
>
> Note .qlit is always QLIT_QNUM(.decoded). Would doing without .qlit
> result in a simpler patch?
Good point. When I wrote this, I mistakenly thought we would end
up having different types of qlits in the array.
I still want to test multiple types of QNums here, not just
QNUM_I64. I will try to get something that is simple but also
gets us more coverage. Maybe as a separate test function and/or
a separate patch.
>
> >> { },
> >> };
> >>
> >> for (i = 0; test_cases[i].encoded; i++) {
> >> QNum *qnum;
> >> int64_t val;
> >> + QNum *qlit_num;
> >> + int64_t qlit_val;
> >>
> >> qnum = qobject_to(QNum,
> >> qobject_from_json(test_cases[i].encoded,
> >> @@ -817,6 +820,7 @@ static void simple_number(void)
> >> g_assert(qnum);
> >> g_assert(qnum_get_try_int(qnum, &val));
> >> g_assert_cmpint(val, ==, test_cases[i].decoded);
> >> +
> >> if (test_cases[i].skip == 0) {
> >> QString *str;
> >>
> >> @@ -826,9 +830,66 @@ static void simple_number(void)
> >> }
> >>
> >> qobject_unref(qnum);
> >> +
> >> + qlit_num = qobject_to(QNum,
> >> + qobject_from_qlit(&test_cases[i].qlit));
> >> + g_assert(qlit_num);
> >> + g_assert(qnum_get_try_int(qlit_num, &qlit_val));
> >> + g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
> >> +
> >> + qobject_unref(qlit_num);
> >> }
> >> }
> >>
[...]
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 5/8] qlit: Support all types of QNums
2020-11-19 18:56 ` Eduardo Habkost
@ 2020-11-20 6:55 ` Markus Armbruster
0 siblings, 0 replies; 54+ messages in thread
From: Markus Armbruster @ 2020-11-20 6:55 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé, QEMU
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Nov 19, 2020 at 11:39:14AM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>> > On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >
>> >> Use QNumValue to represent QNums, so we can also support uint64_t
>> >> and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
>> >> macros for each case.
>> >>
>> >> The QLIT_QNUM() macro is being kept for compatibility with
>> >> existing code, but becomes just a wrapper for QLIT_QNUM_INT().
>> >>
>> >
>> > I am not sure it's worth to keep. (furthermore, it's only used in tests
>> > afaics)
>>
>> Seconded.
>
> Understood, I will remove it. I thought the QAPI code generator
> was using it.
>
> [...]
>> >> diff --git a/qobject/qlit.c b/qobject/qlit.c
>> >> index be8332136c..b23cdc4532 100644
>> >> --- a/qobject/qlit.c
>> >> +++ b/qobject/qlit.c
>> >> @@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
>> >> QObject *rhs)
>> >> case QTYPE_QBOOL:
>> >> return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
>> >> case QTYPE_QNUM:
>> >> - return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
>> >> + return qnum_value_is_equal(&lhs->value.qnum,
>> >> + qnum_get_value(qobject_to(QNum, rhs)));
>>
>> Before the patch, we crash when @rhs can't be represented as int64_t.
>
> I thought it was expected behavior? QLit never supported
> QNUM_U64 or QNUM_DOUBLE as input.
Yes. It's a known limitation, not a bug. You're lifting the
limitation, and that's worth noting in the commit message.
>> Afterwards, we return false (I think).
>>
>> Please note this in the commit message. A separate fix preceding this
>> patch would be even better, but may not be worth the trouble. Up to
>> you.
>
> The fix would be 3 or 4 extra lines of code that would be
> immediately deleted. I'll just mention it as a side effect of
> the new feature.
That's okay.
>> >> case QTYPE_QSTRING:
>> >> return (strcmp(lhs->value.qstr,
>> >> qstring_get_str(qobject_to(QString, rhs))) == 0);
>> >> @@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
>> >> case QTYPE_QNULL:
>> >> return QOBJECT(qnull());
>> >> case QTYPE_QNUM:
>> >> - return QOBJECT(qnum_from_int(qlit->value.qnum));
>> >> + return QOBJECT(qnum_from_value(qlit->value.qnum));
>> >> case QTYPE_QSTRING:
>> >> return QOBJECT(qstring_from_str(qlit->value.qstr));
>> >> case QTYPE_QDICT: {
>> >> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>> >> index 07a773e653..711030cffd 100644
>> >> --- a/tests/check-qjson.c
>> >> +++ b/tests/check-qjson.c
>> >> @@ -796,20 +796,23 @@ static void simple_number(void)
>> >> int i;
>> >> struct {
>> >> const char *encoded;
>> >> + QLitObject qlit;
>> >> int64_t decoded;
>> >> int skip;
>> >> } test_cases[] = {
>> >> - { "0", 0 },
>> >> - { "1234", 1234 },
>> >> - { "1", 1 },
>> >> - { "-32", -32 },
>> >> - { "-0", 0, .skip = 1 },
>> >> + { "0", QLIT_QNUM(0), 0, },
>> >> + { "1234", QLIT_QNUM(1234), 1234, },
>> >> + { "1", QLIT_QNUM(1), 1, },
>> >> + { "-32", QLIT_QNUM(-32), -32, },
>> >> + { "-0", QLIT_QNUM(0), 0, .skip = 1 },
>>
>> Note .qlit is always QLIT_QNUM(.decoded). Would doing without .qlit
>> result in a simpler patch?
>
> Good point. When I wrote this, I mistakenly thought we would end
> up having different types of qlits in the array.
>
> I still want to test multiple types of QNums here, not just
> QNUM_I64. I will try to get something that is simple but also
> gets us more coverage. Maybe as a separate test function and/or
> a separate patch.
Use your judgement :)
[...]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 6/8] qlit: qlit_type() function
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
` (4 preceding siblings ...)
2020-11-16 22:41 ` [PATCH v2 5/8] qlit: Support all types of QNums Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:53 ` Marc-André Lureau
2020-11-19 10:41 ` Markus Armbruster
2020-11-16 22:41 ` [PATCH v2 7/8] qom: Make object_property_set_default() public Eduardo Habkost
` (2 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
Useful function where we need to check for the qlit type before
converting it to an actual QObject.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/qapi/qmp/qlit.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index f9e356d31e..acddb80831 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -59,4 +59,9 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject *rhs);
QObject *qobject_from_qlit(const QLitObject *qlit);
+static inline QType qlit_type(const QLitObject *qlit)
+{
+ return qlit->type;
+}
+
#endif /* QLIT_H */
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 6/8] qlit: qlit_type() function
2020-11-16 22:41 ` [PATCH v2 6/8] qlit: qlit_type() function Eduardo Habkost
@ 2020-11-17 8:53 ` Marc-André Lureau
2020-11-19 10:41 ` Markus Armbruster
1 sibling, 0 replies; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:53 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 900 bytes --]
On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> Useful function where we need to check for the qlit type before
> converting it to an actual QObject.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
> include/qapi/qmp/qlit.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
> index f9e356d31e..acddb80831 100644
> --- a/include/qapi/qmp/qlit.h
> +++ b/include/qapi/qmp/qlit.h
> @@ -59,4 +59,9 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> QObject *rhs);
>
> QObject *qobject_from_qlit(const QLitObject *qlit);
>
> +static inline QType qlit_type(const QLitObject *qlit)
> +{
> + return qlit->type;
> +}
> +
> #endif /* QLIT_H */
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 1626 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 6/8] qlit: qlit_type() function
2020-11-16 22:41 ` [PATCH v2 6/8] qlit: qlit_type() function Eduardo Habkost
2020-11-17 8:53 ` Marc-André Lureau
@ 2020-11-19 10:41 ` Markus Armbruster
2020-11-19 17:56 ` Eduardo Habkost
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-19 10:41 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> Useful function where we need to check for the qlit type before
> converting it to an actual QObject.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> include/qapi/qmp/qlit.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
> index f9e356d31e..acddb80831 100644
> --- a/include/qapi/qmp/qlit.h
> +++ b/include/qapi/qmp/qlit.h
> @@ -59,4 +59,9 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject *rhs);
>
> QObject *qobject_from_qlit(const QLitObject *qlit);
>
> +static inline QType qlit_type(const QLitObject *qlit)
> +{
> + return qlit->type;
> +}
> +
> #endif /* QLIT_H */
Hiding qlit->type behind a function makes sense only when the structure
of QLitObject is an implementation secret. It don't think it is.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 6/8] qlit: qlit_type() function
2020-11-19 10:41 ` Markus Armbruster
@ 2020-11-19 17:56 ` Eduardo Habkost
0 siblings, 0 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 17:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
On Thu, Nov 19, 2020 at 11:41:05AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > Useful function where we need to check for the qlit type before
> > converting it to an actual QObject.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > include/qapi/qmp/qlit.h | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
> > index f9e356d31e..acddb80831 100644
> > --- a/include/qapi/qmp/qlit.h
> > +++ b/include/qapi/qmp/qlit.h
> > @@ -59,4 +59,9 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject *rhs);
> >
> > QObject *qobject_from_qlit(const QLitObject *qlit);
> >
> > +static inline QType qlit_type(const QLitObject *qlit)
> > +{
> > + return qlit->type;
> > +}
> > +
> > #endif /* QLIT_H */
>
> Hiding qlit->type behind a function makes sense only when the structure
> of QLitObject is an implementation secret. It don't think it is.
I thought all QLitObject fields were considered private. If they
are not, I can happily get rid of that function.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 7/8] qom: Make object_property_set_default() public
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
` (5 preceding siblings ...)
2020-11-16 22:41 ` [PATCH v2 6/8] qlit: qlit_type() function Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 8:56 ` Marc-André Lureau
2020-11-16 22:41 ` [PATCH v2 8/8] qom: Use qlit to represent property defaults Eduardo Habkost
2020-11-19 12:39 ` [PATCH v2 0/8] " Markus Armbruster
8 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
The function will be used outside qom/object.c, to simplify the
field property code that sets the property default value.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/qom/object.h | 11 +++++++++++
qom/object.c | 2 +-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 2ab124b8f0..4234cc9b66 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1090,6 +1090,17 @@ ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
ObjectPropertyRelease *release,
void *opaque);
+/**
+ * object_property_set_default:
+ * @prop: the property to set
+ * @value: the value to be written to the property
+ *
+ * Set the property default value.
+ *
+ * Ownership of @value is transferred to the property.
+ */
+void object_property_set_default(ObjectProperty *prop, QObject *value);
+
/**
* object_property_set_default_bool:
* @prop: the property to set
diff --git a/qom/object.c b/qom/object.c
index 7c11bcd3b1..6b0d9d8c79 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1547,7 +1547,7 @@ static void object_property_init_defval(Object *obj, ObjectProperty *prop)
visit_free(v);
}
-static void object_property_set_default(ObjectProperty *prop, QObject *defval)
+void object_property_set_default(ObjectProperty *prop, QObject *defval)
{
assert(!prop->defval);
assert(!prop->init);
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 7/8] qom: Make object_property_set_default() public
2020-11-16 22:41 ` [PATCH v2 7/8] qom: Make object_property_set_default() public Eduardo Habkost
@ 2020-11-17 8:56 ` Marc-André Lureau
0 siblings, 0 replies; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 8:56 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]
On Tue, Nov 17, 2020 at 2:44 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> The function will be used outside qom/object.c, to simplify the
> field property code that sets the property default value.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
> include/qom/object.h | 11 +++++++++++
> qom/object.c | 2 +-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 2ab124b8f0..4234cc9b66 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1090,6 +1090,17 @@ ObjectProperty
> *object_class_property_add(ObjectClass *klass, const char *name,
> ObjectPropertyRelease *release,
> void *opaque);
>
> +/**
> + * object_property_set_default:
> + * @prop: the property to set
> + * @value: the value to be written to the property
> + *
> + * Set the property default value.
> + *
> + * Ownership of @value is transferred to the property.
> + */
> +void object_property_set_default(ObjectProperty *prop, QObject *value);
> +
> /**
> * object_property_set_default_bool:
> * @prop: the property to set
> diff --git a/qom/object.c b/qom/object.c
> index 7c11bcd3b1..6b0d9d8c79 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1547,7 +1547,7 @@ static void object_property_init_defval(Object *obj,
> ObjectProperty *prop)
> visit_free(v);
> }
>
> -static void object_property_set_default(ObjectProperty *prop, QObject
> *defval)
> +void object_property_set_default(ObjectProperty *prop, QObject *defval)
> {
> assert(!prop->defval);
> assert(!prop->init);
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 2670 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 8/8] qom: Use qlit to represent property defaults
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
` (6 preceding siblings ...)
2020-11-16 22:41 ` [PATCH v2 7/8] qom: Make object_property_set_default() public Eduardo Habkost
@ 2020-11-16 22:41 ` Eduardo Habkost
2020-11-17 9:02 ` Marc-André Lureau
2020-11-19 12:39 ` [PATCH v2 0/8] " Markus Armbruster
8 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-16 22:41 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster
Using QLitObject lets us get rid of most of the
.set_default_value functions, and just use
object_property_set_default() directly.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Instead of initializing defval to QLIT_QNULL
by default, just check for QTYPE_NONE, to find out if .defval
was explicitly set. This avoids extra complexity at
set_prop_arraylen().
---
include/hw/qdev-properties-system.h | 2 +-
include/qom/field-property-internal.h | 4 ---
include/qom/field-property.h | 26 ++++++++-----------
include/qom/property-types.h | 19 ++++++--------
hw/core/qdev-properties-system.c | 8 ------
qom/field-property.c | 27 ++++++++++++++------
qom/property-types.c | 36 ++++-----------------------
7 files changed, 42 insertions(+), 80 deletions(-)
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 0ac327ae60..a586424a33 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -65,7 +65,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
#define DEFINE_PROP_UUID(_name, _state, _field) \
DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID, \
- .set_default = true)
+ .defval = QLIT_QSTR("auto"))
#define DEFINE_PROP_AUDIODEV(_n, _s, _f) \
DEFINE_PROP(_n, _s, _f, qdev_prop_audiodev, QEMUSoundCard)
diff --git a/include/qom/field-property-internal.h b/include/qom/field-property-internal.h
index a7b7e2b69d..9bc29e9b67 100644
--- a/include/qom/field-property-internal.h
+++ b/include/qom/field-property-internal.h
@@ -15,10 +15,6 @@ void field_prop_set_enum(Object *obj, Visitor *v, const char *name,
void field_prop_set_default_value_enum(ObjectProperty *op,
const Property *prop);
-void field_prop_set_default_value_int(ObjectProperty *op,
- const Property *prop);
-void field_prop_set_default_value_uint(ObjectProperty *op,
- const Property *prop);
void field_prop_get_int32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp);
diff --git a/include/qom/field-property.h b/include/qom/field-property.h
index 0cb1fe2217..3cfd19cc14 100644
--- a/include/qom/field-property.h
+++ b/include/qom/field-property.h
@@ -6,6 +6,7 @@
#include "qom/object.h"
#include "qapi/util.h"
+#include "qapi/qmp/qlit.h"
/**
* struct Property: definition of a field property
@@ -27,21 +28,8 @@ struct Property {
const PropertyInfo *info;
ptrdiff_t offset;
uint8_t bitnr;
- /**
- * @set_default: true if the default value should be set from @defval,
- * in which case @info->set_default_value must not be NULL
- * (if false then no default value is set by the property system
- * and the field retains whatever value it was given by instance_init).
- */
- bool set_default;
- /**
- * @defval: default value for the property. This is used only if @set_default
- * is true.
- */
- union {
- int64_t i;
- uint64_t u;
- } defval;
+ /** @defval: If not QTYPE_NONE, the default value for the property */
+ QLitObject defval;
/* private: */
int arrayoffset;
const PropertyInfo *arrayinfo;
@@ -61,7 +49,13 @@ struct PropertyInfo {
const QEnumLookup *enum_table;
/** @print: String formatting function, for the human monitor */
int (*print)(Object *obj, Property *prop, char *dest, size_t len);
- /** @set_default_value: Callback for initializing the default value */
+ /**
+ * @set_default_value: Optional callback for initializing the default value
+ *
+ * Most property types don't need to set this, as by default
+ * object_property_set_default() is called with the value at
+ * Property.defval.
+ */
void (*set_default_value)(ObjectProperty *op, const Property *prop);
/** @create: Optional callback for creation of property */
ObjectProperty *(*create)(ObjectClass *oc, const char *name,
diff --git a/include/qom/property-types.h b/include/qom/property-types.h
index 3132ddafd9..869d1a993a 100644
--- a/include/qom/property-types.h
+++ b/include/qom/property-types.h
@@ -5,6 +5,7 @@
#define QOM_PROPERTY_TYPES_H
#include "qom/field-property.h"
+#include "qapi/qmp/qlit.h"
extern const PropertyInfo prop_info_bit;
extern const PropertyInfo prop_info_bit64;
@@ -25,34 +26,29 @@ extern const PropertyInfo prop_info_link;
#define PROP_SIGNED(_state, _field, _defval, _prop, _type, ...) \
FIELD_PROP(_state, _field, _prop, _type, \
- .set_default = true, \
- .defval.i = (_type)_defval, \
+ .defval = QLIT_QNUM_INT((_type)_defval), \
__VA_ARGS__)
#define PROP_UNSIGNED(_state, _field, _defval, _prop, _type, ...) \
FIELD_PROP(_state, _field, _prop, _type, \
- .set_default = true, \
- .defval.u = (_type)_defval, \
+ .defval = QLIT_QNUM_UINT((_type)_defval), \
__VA_ARGS__)
#define PROP_BIT(_state, _field, _bit, _defval, ...) \
FIELD_PROP(_state, _field, prop_info_bit, uint32_t, \
.bitnr = (_bit), \
- .set_default = true, \
- .defval.u = (bool)_defval, \
+ .defval = QLIT_QBOOL(_defval), \
__VA_ARGS__)
#define PROP_BIT64(_state, _field, _bit, _defval, ...) \
FIELD_PROP(_state, _field, prop_info_bit64, uint64_t, \
.bitnr = (_bit), \
- .set_default = true, \
- .defval.u = (bool)_defval, \
+ .defval = QLIT_QBOOL(_defval), \
__VA_ARGS__)
#define PROP_BOOL(_state, _field, _defval, ...) \
FIELD_PROP(_state, _field, prop_info_bool, bool, \
- .set_default = true, \
- .defval.u = (bool)_defval, \
+ .defval = QLIT_QBOOL(_defval), \
__VA_ARGS__)
#define PROP_LINK(_state, _field, _type, _ptr_type, ...) \
@@ -131,8 +127,7 @@ extern const PropertyInfo prop_info_link;
_arrayfield, _arrayprop, _arraytype) \
DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name), \
_state, _field, prop_info_arraylen, uint32_t, \
- .set_default = true, \
- .defval.u = 0, \
+ .defval = QLIT_QNUM_UINT(0), \
.arrayinfo = &(_arrayprop), \
.arrayfieldsize = sizeof(_arraytype), \
.arrayoffset = offsetof(_state, _arrayfield))
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 8da68f076c..d9be5372f6 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -570,7 +570,6 @@ const PropertyInfo qdev_prop_blocksize = {
" and " MAX_BLOCK_SIZE_STR,
.get = field_prop_get_size32,
.set = set_blocksize,
- .set_default_value = field_prop_set_default_value_uint,
};
/* --- Block device error handling policy --- */
@@ -768,7 +767,6 @@ const PropertyInfo qdev_prop_pci_devfn = {
.print = print_pci_devfn,
.get = field_prop_get_int32,
.set = set_pci_devfn,
- .set_default_value = field_prop_set_default_value_int,
};
/* --- pci host address --- */
@@ -1080,16 +1078,10 @@ static void set_uuid(Object *obj, Visitor *v, const char *name, void *opaque,
g_free(str);
}
-static void set_default_uuid_auto(ObjectProperty *op, const Property *prop)
-{
- object_property_set_default_str(op, UUID_VALUE_AUTO);
-}
-
const PropertyInfo qdev_prop_uuid = {
.name = "str",
.description = "UUID (aka GUID) or \"" UUID_VALUE_AUTO
"\" for random value (default)",
.get = get_uuid,
.set = set_uuid,
- .set_default_value = set_default_uuid_auto,
};
diff --git a/qom/field-property.c b/qom/field-property.c
index cb729626ce..9cb5ded41a 100644
--- a/qom/field-property.c
+++ b/qom/field-property.c
@@ -47,6 +47,20 @@ static ObjectPropertyAccessor *field_prop_setter(const PropertyInfo *info)
return info->set ? field_prop_set : NULL;
}
+static void field_prop_set_default_value(ObjectProperty *op,
+ Property *prop)
+{
+ if (qlit_type(&prop->defval) == QTYPE_NONE) {
+ return;
+ }
+
+ if (prop->info->set_default_value) {
+ prop->info->set_default_value(op, prop);
+ } else {
+ object_property_set_default(op, qobject_from_qlit(&prop->defval));
+ }
+}
+
/*
* Property release callback for dynamically-created properties:
* We call the underlying element's property release hook, and
@@ -83,11 +97,9 @@ object_property_add_field(Object *obj, const char *name,
object_property_set_description(obj, name,
newprop->info->description);
- if (newprop->set_default) {
- newprop->info->set_default_value(op, newprop);
- if (op->init) {
- op->init(obj, op);
- }
+ field_prop_set_default_value(op, prop);
+ if (op->init) {
+ op->init(obj, op);
}
op->allow_set = allow_set;
@@ -113,9 +125,8 @@ object_class_property_add_field_static(ObjectClass *oc, const char *name,
prop->info->release,
prop);
}
- if (prop->set_default) {
- prop->info->set_default_value(op, prop);
- }
+
+ field_prop_set_default_value(op, prop);
if (prop->info->description) {
object_class_property_set_description(oc, name,
prop->info->description);
diff --git a/qom/property-types.c b/qom/property-types.c
index 4b3fe15b6a..fe96f1f49a 100644
--- a/qom/property-types.c
+++ b/qom/property-types.c
@@ -28,8 +28,11 @@ void field_prop_set_enum(Object *obj, Visitor *v, const char *name,
void field_prop_set_default_value_enum(ObjectProperty *op,
const Property *prop)
{
- object_property_set_default_str(op,
- qapi_enum_lookup(prop->info->enum_table, prop->defval.i));
+ QObject *defval = qobject_from_qlit(&prop->defval);
+ const char *str = qapi_enum_lookup(prop->info->enum_table,
+ qnum_get_int(qobject_to(QNum, defval)));
+ object_property_set_default_str(op, str);
+ qobject_unref(defval);
}
const PropertyInfo prop_info_enum = {
@@ -80,17 +83,11 @@ static void prop_set_bit(Object *obj, Visitor *v, const char *name,
bit_prop_set(obj, prop, value);
}
-static void set_default_value_bool(ObjectProperty *op, const Property *prop)
-{
- object_property_set_default_bool(op, prop->defval.u);
-}
-
const PropertyInfo prop_info_bit = {
.name = "bool",
.description = "on/off",
.get = prop_get_bit,
.set = prop_set_bit,
- .set_default_value = set_default_value_bool,
};
/* Bit64 */
@@ -139,7 +136,6 @@ const PropertyInfo prop_info_bit64 = {
.description = "on/off",
.get = prop_get_bit64,
.set = prop_set_bit64,
- .set_default_value = set_default_value_bool,
};
/* --- bool --- */
@@ -166,7 +162,6 @@ const PropertyInfo prop_info_bool = {
.name = "bool",
.get = get_bool,
.set = set_bool,
- .set_default_value = set_default_value_bool,
};
/* --- 8bit integer --- */
@@ -189,23 +184,10 @@ static void set_uint8(Object *obj, Visitor *v, const char *name, void *opaque,
visit_type_uint8(v, name, ptr, errp);
}
-void field_prop_set_default_value_int(ObjectProperty *op,
- const Property *prop)
-{
- object_property_set_default_int(op, prop->defval.i);
-}
-
-void field_prop_set_default_value_uint(ObjectProperty *op,
- const Property *prop)
-{
- object_property_set_default_uint(op, prop->defval.u);
-}
-
const PropertyInfo prop_info_uint8 = {
.name = "uint8",
.get = get_uint8,
.set = set_uint8,
- .set_default_value = field_prop_set_default_value_uint,
};
/* --- 16bit integer --- */
@@ -232,7 +214,6 @@ const PropertyInfo prop_info_uint16 = {
.name = "uint16",
.get = get_uint16,
.set = set_uint16,
- .set_default_value = field_prop_set_default_value_uint,
};
/* --- 32bit integer --- */
@@ -277,14 +258,12 @@ const PropertyInfo prop_info_uint32 = {
.name = "uint32",
.get = get_uint32,
.set = set_uint32,
- .set_default_value = field_prop_set_default_value_uint,
};
const PropertyInfo prop_info_int32 = {
.name = "int32",
.get = field_prop_get_int32,
.set = set_int32,
- .set_default_value = field_prop_set_default_value_int,
};
/* --- 64bit integer --- */
@@ -329,14 +308,12 @@ const PropertyInfo prop_info_uint64 = {
.name = "uint64",
.get = get_uint64,
.set = set_uint64,
- .set_default_value = field_prop_set_default_value_uint,
};
const PropertyInfo prop_info_int64 = {
.name = "int64",
.get = get_int64,
.set = set_int64,
- .set_default_value = field_prop_set_default_value_int,
};
/* --- string --- */
@@ -431,7 +408,6 @@ const PropertyInfo prop_info_size32 = {
.name = "size",
.get = field_prop_get_size32,
.set = set_size32,
- .set_default_value = field_prop_set_default_value_uint,
};
/* --- support for array properties --- */
@@ -495,7 +471,6 @@ const PropertyInfo prop_info_arraylen = {
.name = "uint32",
.get = get_uint32,
.set = set_prop_arraylen,
- .set_default_value = field_prop_set_default_value_uint,
};
/* --- 64bit unsigned int 'size' type --- */
@@ -522,7 +497,6 @@ const PropertyInfo prop_info_size = {
.name = "size",
.get = get_size,
.set = set_size,
- .set_default_value = field_prop_set_default_value_uint,
};
/* --- object link property --- */
--
2.28.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2 8/8] qom: Use qlit to represent property defaults
2020-11-16 22:41 ` [PATCH v2 8/8] qom: Use qlit to represent property defaults Eduardo Habkost
@ 2020-11-17 9:02 ` Marc-André Lureau
0 siblings, 0 replies; 54+ messages in thread
From: Marc-André Lureau @ 2020-11-17 9:02 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Daniel P. Berrangé, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 15964 bytes --]
On Tue, Nov 17, 2020 at 2:45 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> Using QLitObject lets us get rid of most of the
> .set_default_value functions, and just use
> object_property_set_default() directly.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
> Changes v1 -> v2:
> * Instead of initializing defval to QLIT_QNULL
> by default, just check for QTYPE_NONE, to find out if .defval
> was explicitly set. This avoids extra complexity at
> set_prop_arraylen().
> ---
> include/hw/qdev-properties-system.h | 2 +-
> include/qom/field-property-internal.h | 4 ---
> include/qom/field-property.h | 26 ++++++++-----------
> include/qom/property-types.h | 19 ++++++--------
> hw/core/qdev-properties-system.c | 8 ------
> qom/field-property.c | 27 ++++++++++++++------
> qom/property-types.c | 36 ++++-----------------------
> 7 files changed, 42 insertions(+), 80 deletions(-)
>
> diff --git a/include/hw/qdev-properties-system.h
> b/include/hw/qdev-properties-system.h
> index 0ac327ae60..a586424a33 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -65,7 +65,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>
> #define DEFINE_PROP_UUID(_name, _state, _field) \
> DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID, \
> - .set_default = true)
> + .defval = QLIT_QSTR("auto"))
>
> #define DEFINE_PROP_AUDIODEV(_n, _s, _f) \
> DEFINE_PROP(_n, _s, _f, qdev_prop_audiodev, QEMUSoundCard)
> diff --git a/include/qom/field-property-internal.h
> b/include/qom/field-property-internal.h
> index a7b7e2b69d..9bc29e9b67 100644
> --- a/include/qom/field-property-internal.h
> +++ b/include/qom/field-property-internal.h
> @@ -15,10 +15,6 @@ void field_prop_set_enum(Object *obj, Visitor *v, const
> char *name,
>
> void field_prop_set_default_value_enum(ObjectProperty *op,
> const Property *prop);
> -void field_prop_set_default_value_int(ObjectProperty *op,
> - const Property *prop);
> -void field_prop_set_default_value_uint(ObjectProperty *op,
> - const Property *prop);
>
> void field_prop_get_int32(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp);
> diff --git a/include/qom/field-property.h b/include/qom/field-property.h
> index 0cb1fe2217..3cfd19cc14 100644
> --- a/include/qom/field-property.h
> +++ b/include/qom/field-property.h
> @@ -6,6 +6,7 @@
>
> #include "qom/object.h"
> #include "qapi/util.h"
> +#include "qapi/qmp/qlit.h"
>
> /**
> * struct Property: definition of a field property
> @@ -27,21 +28,8 @@ struct Property {
> const PropertyInfo *info;
> ptrdiff_t offset;
> uint8_t bitnr;
> - /**
> - * @set_default: true if the default value should be set from @defval,
> - * in which case @info->set_default_value must not be NULL
> - * (if false then no default value is set by the property system
> - * and the field retains whatever value it was given by
> instance_init).
> - */
> - bool set_default;
> - /**
> - * @defval: default value for the property. This is used only if
> @set_default
> - * is true.
> - */
> - union {
> - int64_t i;
> - uint64_t u;
> - } defval;
> + /** @defval: If not QTYPE_NONE, the default value for the property */
> + QLitObject defval;
> /* private: */
> int arrayoffset;
> const PropertyInfo *arrayinfo;
> @@ -61,7 +49,13 @@ struct PropertyInfo {
> const QEnumLookup *enum_table;
> /** @print: String formatting function, for the human monitor */
> int (*print)(Object *obj, Property *prop, char *dest, size_t len);
> - /** @set_default_value: Callback for initializing the default value */
> + /**
> + * @set_default_value: Optional callback for initializing the default
> value
> + *
> + * Most property types don't need to set this, as by default
> + * object_property_set_default() is called with the value at
> + * Property.defval.
> + */
> void (*set_default_value)(ObjectProperty *op, const Property *prop);
> /** @create: Optional callback for creation of property */
> ObjectProperty *(*create)(ObjectClass *oc, const char *name,
> diff --git a/include/qom/property-types.h b/include/qom/property-types.h
> index 3132ddafd9..869d1a993a 100644
> --- a/include/qom/property-types.h
> +++ b/include/qom/property-types.h
> @@ -5,6 +5,7 @@
> #define QOM_PROPERTY_TYPES_H
>
> #include "qom/field-property.h"
> +#include "qapi/qmp/qlit.h"
>
> extern const PropertyInfo prop_info_bit;
> extern const PropertyInfo prop_info_bit64;
> @@ -25,34 +26,29 @@ extern const PropertyInfo prop_info_link;
>
> #define PROP_SIGNED(_state, _field, _defval, _prop, _type, ...) \
> FIELD_PROP(_state, _field, _prop, _type, \
> - .set_default = true, \
> - .defval.i = (_type)_defval, \
> + .defval = QLIT_QNUM_INT((_type)_defval), \
> __VA_ARGS__)
>
> #define PROP_UNSIGNED(_state, _field, _defval, _prop, _type, ...) \
> FIELD_PROP(_state, _field, _prop, _type, \
> - .set_default = true, \
> - .defval.u = (_type)_defval, \
> + .defval = QLIT_QNUM_UINT((_type)_defval), \
> __VA_ARGS__)
>
> #define PROP_BIT(_state, _field, _bit, _defval, ...) \
> FIELD_PROP(_state, _field, prop_info_bit, uint32_t, \
> .bitnr = (_bit), \
> - .set_default = true, \
> - .defval.u = (bool)_defval, \
> + .defval = QLIT_QBOOL(_defval), \
> __VA_ARGS__)
>
> #define PROP_BIT64(_state, _field, _bit, _defval, ...) \
> FIELD_PROP(_state, _field, prop_info_bit64, uint64_t, \
> .bitnr = (_bit), \
> - .set_default = true, \
> - .defval.u = (bool)_defval, \
> + .defval = QLIT_QBOOL(_defval), \
> __VA_ARGS__)
>
> #define PROP_BOOL(_state, _field, _defval, ...) \
> FIELD_PROP(_state, _field, prop_info_bool, bool, \
> - .set_default = true, \
> - .defval.u = (bool)_defval, \
> + .defval = QLIT_QBOOL(_defval), \
> __VA_ARGS__)
>
> #define PROP_LINK(_state, _field, _type, _ptr_type, ...) \
> @@ -131,8 +127,7 @@ extern const PropertyInfo prop_info_link;
> _arrayfield, _arrayprop, _arraytype) \
> DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name), \
> _state, _field, prop_info_arraylen, uint32_t, \
> - .set_default = true, \
> - .defval.u = 0, \
> + .defval = QLIT_QNUM_UINT(0), \
> .arrayinfo = &(_arrayprop), \
> .arrayfieldsize = sizeof(_arraytype), \
> .arrayoffset = offsetof(_state, _arrayfield))
> diff --git a/hw/core/qdev-properties-system.c
> b/hw/core/qdev-properties-system.c
> index 8da68f076c..d9be5372f6 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -570,7 +570,6 @@ const PropertyInfo qdev_prop_blocksize = {
> " and " MAX_BLOCK_SIZE_STR,
> .get = field_prop_get_size32,
> .set = set_blocksize,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> /* --- Block device error handling policy --- */
> @@ -768,7 +767,6 @@ const PropertyInfo qdev_prop_pci_devfn = {
> .print = print_pci_devfn,
> .get = field_prop_get_int32,
> .set = set_pci_devfn,
> - .set_default_value = field_prop_set_default_value_int,
> };
>
> /* --- pci host address --- */
> @@ -1080,16 +1078,10 @@ static void set_uuid(Object *obj, Visitor *v,
> const char *name, void *opaque,
> g_free(str);
> }
>
> -static void set_default_uuid_auto(ObjectProperty *op, const Property
> *prop)
> -{
> - object_property_set_default_str(op, UUID_VALUE_AUTO);
> -}
> -
> const PropertyInfo qdev_prop_uuid = {
> .name = "str",
> .description = "UUID (aka GUID) or \"" UUID_VALUE_AUTO
> "\" for random value (default)",
> .get = get_uuid,
> .set = set_uuid,
> - .set_default_value = set_default_uuid_auto,
> };
> diff --git a/qom/field-property.c b/qom/field-property.c
> index cb729626ce..9cb5ded41a 100644
> --- a/qom/field-property.c
> +++ b/qom/field-property.c
> @@ -47,6 +47,20 @@ static ObjectPropertyAccessor *field_prop_setter(const
> PropertyInfo *info)
> return info->set ? field_prop_set : NULL;
> }
>
> +static void field_prop_set_default_value(ObjectProperty *op,
> + Property *prop)
> +{
> + if (qlit_type(&prop->defval) == QTYPE_NONE) {
> + return;
> + }
> +
> + if (prop->info->set_default_value) {
> + prop->info->set_default_value(op, prop);
> + } else {
> + object_property_set_default(op, qobject_from_qlit(&prop->defval));
> + }
> +}
> +
> /*
> * Property release callback for dynamically-created properties:
> * We call the underlying element's property release hook, and
> @@ -83,11 +97,9 @@ object_property_add_field(Object *obj, const char *name,
> object_property_set_description(obj, name,
> newprop->info->description);
>
> - if (newprop->set_default) {
> - newprop->info->set_default_value(op, newprop);
> - if (op->init) {
> - op->init(obj, op);
> - }
> + field_prop_set_default_value(op, prop);
> + if (op->init) {
> + op->init(obj, op);
> }
>
> op->allow_set = allow_set;
> @@ -113,9 +125,8 @@ object_class_property_add_field_static(ObjectClass
> *oc, const char *name,
> prop->info->release,
> prop);
> }
> - if (prop->set_default) {
> - prop->info->set_default_value(op, prop);
> - }
> +
> + field_prop_set_default_value(op, prop);
> if (prop->info->description) {
> object_class_property_set_description(oc, name,
> prop->info->description);
> diff --git a/qom/property-types.c b/qom/property-types.c
> index 4b3fe15b6a..fe96f1f49a 100644
> --- a/qom/property-types.c
> +++ b/qom/property-types.c
> @@ -28,8 +28,11 @@ void field_prop_set_enum(Object *obj, Visitor *v, const
> char *name,
> void field_prop_set_default_value_enum(ObjectProperty *op,
> const Property *prop)
> {
> - object_property_set_default_str(op,
> - qapi_enum_lookup(prop->info->enum_table, prop->defval.i));
> + QObject *defval = qobject_from_qlit(&prop->defval);
> + const char *str = qapi_enum_lookup(prop->info->enum_table,
> + qnum_get_int(qobject_to(QNum,
> defval)));
> + object_property_set_default_str(op, str);
> + qobject_unref(defval);
> }
>
> const PropertyInfo prop_info_enum = {
> @@ -80,17 +83,11 @@ static void prop_set_bit(Object *obj, Visitor *v,
> const char *name,
> bit_prop_set(obj, prop, value);
> }
>
> -static void set_default_value_bool(ObjectProperty *op, const Property
> *prop)
> -{
> - object_property_set_default_bool(op, prop->defval.u);
> -}
> -
> const PropertyInfo prop_info_bit = {
> .name = "bool",
> .description = "on/off",
> .get = prop_get_bit,
> .set = prop_set_bit,
> - .set_default_value = set_default_value_bool,
> };
>
> /* Bit64 */
> @@ -139,7 +136,6 @@ const PropertyInfo prop_info_bit64 = {
> .description = "on/off",
> .get = prop_get_bit64,
> .set = prop_set_bit64,
> - .set_default_value = set_default_value_bool,
> };
>
> /* --- bool --- */
> @@ -166,7 +162,6 @@ const PropertyInfo prop_info_bool = {
> .name = "bool",
> .get = get_bool,
> .set = set_bool,
> - .set_default_value = set_default_value_bool,
> };
>
> /* --- 8bit integer --- */
> @@ -189,23 +184,10 @@ static void set_uint8(Object *obj, Visitor *v, const
> char *name, void *opaque,
> visit_type_uint8(v, name, ptr, errp);
> }
>
> -void field_prop_set_default_value_int(ObjectProperty *op,
> - const Property *prop)
> -{
> - object_property_set_default_int(op, prop->defval.i);
> -}
> -
> -void field_prop_set_default_value_uint(ObjectProperty *op,
> - const Property *prop)
> -{
> - object_property_set_default_uint(op, prop->defval.u);
> -}
> -
> const PropertyInfo prop_info_uint8 = {
> .name = "uint8",
> .get = get_uint8,
> .set = set_uint8,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> /* --- 16bit integer --- */
> @@ -232,7 +214,6 @@ const PropertyInfo prop_info_uint16 = {
> .name = "uint16",
> .get = get_uint16,
> .set = set_uint16,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> /* --- 32bit integer --- */
> @@ -277,14 +258,12 @@ const PropertyInfo prop_info_uint32 = {
> .name = "uint32",
> .get = get_uint32,
> .set = set_uint32,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> const PropertyInfo prop_info_int32 = {
> .name = "int32",
> .get = field_prop_get_int32,
> .set = set_int32,
> - .set_default_value = field_prop_set_default_value_int,
> };
>
> /* --- 64bit integer --- */
> @@ -329,14 +308,12 @@ const PropertyInfo prop_info_uint64 = {
> .name = "uint64",
> .get = get_uint64,
> .set = set_uint64,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> const PropertyInfo prop_info_int64 = {
> .name = "int64",
> .get = get_int64,
> .set = set_int64,
> - .set_default_value = field_prop_set_default_value_int,
> };
>
> /* --- string --- */
> @@ -431,7 +408,6 @@ const PropertyInfo prop_info_size32 = {
> .name = "size",
> .get = field_prop_get_size32,
> .set = set_size32,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> /* --- support for array properties --- */
> @@ -495,7 +471,6 @@ const PropertyInfo prop_info_arraylen = {
> .name = "uint32",
> .get = get_uint32,
> .set = set_prop_arraylen,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> /* --- 64bit unsigned int 'size' type --- */
> @@ -522,7 +497,6 @@ const PropertyInfo prop_info_size = {
> .name = "size",
> .get = get_size,
> .set = set_size,
> - .set_default_value = field_prop_set_default_value_uint,
> };
>
> /* --- object link property --- */
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 19243 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
2020-11-16 22:41 [PATCH v2 0/8] qom: Use qlit to represent property defaults Eduardo Habkost
` (7 preceding siblings ...)
2020-11-16 22:41 ` [PATCH v2 8/8] qom: Use qlit to represent property defaults Eduardo Habkost
@ 2020-11-19 12:39 ` Markus Armbruster
2020-11-19 17:13 ` Eduardo Habkost
8 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2020-11-19 12:39 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> Based-on: 20201104160021.2342108-1-ehabkost@redhat.com
> Git branch: https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-qlit-defaults
>
> This extend qlit.h to support all QNum types (signed int,
> unsigned int, and double), and use QLitObject to represent field
> property defaults.
>
> It allows us to get rid of most type-specific .set_default_value
> functions for QOM property types.
What's left?
I'm asking because if you create a new way to get rid of most of an old
way, you're still left with two ways, which may or may not be an
improvement.
Moving defaults from code to data sounds attractive to me. Data is
easier to reason about than code. For QAPI, we've been talking about
defining defaults in the schema for a long time, but nobody has gotten
around to finish an implementation.
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
2020-11-19 12:39 ` [PATCH v2 0/8] " Markus Armbruster
@ 2020-11-19 17:13 ` Eduardo Habkost
2020-11-19 17:23 ` Paolo Bonzini
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 17:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
On Thu, Nov 19, 2020 at 01:39:50PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > Based-on: 20201104160021.2342108-1-ehabkost@redhat.com
> > Git branch: https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-qlit-defaults
> >
> > This extend qlit.h to support all QNum types (signed int,
> > unsigned int, and double), and use QLitObject to represent field
> > property defaults.
> >
> > It allows us to get rid of most type-specific .set_default_value
> > functions for QOM property types.
>
> What's left?
Enums. Enums properties are a mess to implement, and I plan to
tackle them later.
On all other cases, the external representation of the property
value is similar to the internal representation. In the case of
enums, the external representation is a string, but the internal
representation is an integer.
>
> I'm asking because if you create a new way to get rid of most of an old
> way, you're still left with two ways, which may or may not be an
> improvement.
I don't think this is an accurate description. We had five
different ways of doing this[1]. I replaced four of them with
the new qlit-based mechanism, and now we have two.
[1] The five different ways were:
qdev_propinfo_set_default_value_enum,
qdev_propinfo_set_default_value_int,
qdev_propinfo_set_default_value_uint,
set_default_uuid_auto, and
set_default_value_bool.
>
> Moving defaults from code to data sounds attractive to me. Data is
> easier to reason about than code. For QAPI, we've been talking about
> defining defaults in the schema for a long time, but nobody has gotten
> around to finish an implementation.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
2020-11-19 17:13 ` Eduardo Habkost
@ 2020-11-19 17:23 ` Paolo Bonzini
2020-11-19 17:55 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2020-11-19 17:23 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster; +Cc: Daniel P. Berrangé, qemu-devel
On 19/11/20 18:13, Eduardo Habkost wrote:
>> What's left?
> Enums. Enums properties are a mess to implement, and I plan to
> tackle them later.
>
> On all other cases, the external representation of the property
> value is similar to the internal representation. In the case of
> enums, the external representation is a string, but the internal
> representation is an integer.
>
I would have expected a string QLit to work with enums, is there a
reason why it doesn't?
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
2020-11-19 17:23 ` Paolo Bonzini
@ 2020-11-19 17:55 ` Eduardo Habkost
2020-11-19 18:25 ` Paolo Bonzini
0 siblings, 1 reply; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 17:55 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Daniel P. Berrangé, Markus Armbruster, qemu-devel
On Thu, Nov 19, 2020 at 06:23:30PM +0100, Paolo Bonzini wrote:
> On 19/11/20 18:13, Eduardo Habkost wrote:
> > > What's left?
> > Enums. Enums properties are a mess to implement, and I plan to
> > tackle them later.
> >
> > On all other cases, the external representation of the property
> > value is similar to the internal representation. In the case of
> > enums, the external representation is a string, but the internal
> > representation is an integer.
> >
>
> I would have expected a string QLit to work with enums, is there a reason
> why it doesn't?
It would work, but it would be more inconvenient for callers.
Right now they use the C enum constant instead of a string.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
2020-11-19 17:55 ` Eduardo Habkost
@ 2020-11-19 18:25 ` Paolo Bonzini
2020-11-19 19:05 ` Eduardo Habkost
0 siblings, 1 reply; 54+ messages in thread
From: Paolo Bonzini @ 2020-11-19 18:25 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Daniel P. Berrangé, Markus Armbruster, qemu-devel
On 19/11/20 18:55, Eduardo Habkost wrote:
> On Thu, Nov 19, 2020 at 06:23:30PM +0100, Paolo Bonzini wrote:
>> On 19/11/20 18:13, Eduardo Habkost wrote:
>>>> What's left?
>>> Enums. Enums properties are a mess to implement, and I plan to
>>> tackle them later.
>>>
>>> On all other cases, the external representation of the property
>>> value is similar to the internal representation. In the case of
>>> enums, the external representation is a string, but the internal
>>> representation is an integer.
>>>
>>
>> I would have expected a string QLit to work with enums, is there a reason
>> why it doesn't?
>
> It would work, but it would be more inconvenient for callers.
> Right now they use the C enum constant instead of a string.
It matches what you have to do already for compat props, so it's not a
big deal. I would say just use strings.
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
2020-11-19 18:25 ` Paolo Bonzini
@ 2020-11-19 19:05 ` Eduardo Habkost
0 siblings, 0 replies; 54+ messages in thread
From: Eduardo Habkost @ 2020-11-19 19:05 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Daniel P. Berrangé, Markus Armbruster, qemu-devel
On Thu, Nov 19, 2020 at 07:25:15PM +0100, Paolo Bonzini wrote:
> On 19/11/20 18:55, Eduardo Habkost wrote:
> > On Thu, Nov 19, 2020 at 06:23:30PM +0100, Paolo Bonzini wrote:
> > > On 19/11/20 18:13, Eduardo Habkost wrote:
> > > > > What's left?
> > > > Enums. Enums properties are a mess to implement, and I plan to
> > > > tackle them later.
> > > >
> > > > On all other cases, the external representation of the property
> > > > value is similar to the internal representation. In the case of
> > > > enums, the external representation is a string, but the internal
> > > > representation is an integer.
> > > >
> > >
> > > I would have expected a string QLit to work with enums, is there a reason
> > > why it doesn't?
> >
> > It would work, but it would be more inconvenient for callers.
> > Right now they use the C enum constant instead of a string.
>
> It matches what you have to do already for compat props, so it's not a big
> deal. I would say just use strings.
No problem to me. I'll do.
--
Eduardo
^ permalink raw reply [flat|nested] 54+ messages in thread