Patchwork [02/29] Introduce QInt

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 19, 2009, 11:07 p.m.
Message ID <1250723280-3509-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/31675/
State Superseded
Headers show

Comments

Luiz Capitulino - Aug. 19, 2009, 11:07 p.m.
QInt is a high-level data type that can be used to store integers
and perform type-safe conversions.

The following functions are available:

- qint_from_int()   Create a new QInt from an int
- qint_from_int64() Create a new QInt from an int64_t
- qint_to_int()     Export QInt to int
- qint_to_uint64()  Export QInt to uint64_t
- qint_to_uint32()  Export QInt to uint32_t

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile  |    1 +
 qint.c    |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qint.h    |   20 +++++++++++++
 qobject.h |    1 +
 4 files changed, 114 insertions(+), 0 deletions(-)
 create mode 100644 qint.c
 create mode 100644 qint.h
Avi Kivity - Aug. 20, 2009, 7:51 a.m.
On 08/20/2009 02:07 AM, Luiz Capitulino wrote:
> QInt is a high-level data type that can be used to store integers
> and perform type-safe conversions.
>
> The following functions are available:
>
> - qint_from_int()   Create a new QInt from an int
> - qint_from_int64() Create a new QInt from an int64_t
> - qint_to_int()     Export QInt to int
> - qint_to_uint64()  Export QInt to uint64_t
> - qint_to_uint32()  Export QInt to uint32_t
>    

Why aren't the conversion functions symmetrical?
Paolo Bonzini - Aug. 20, 2009, 8:37 a.m.
On 08/20/2009 01:07 AM, Luiz Capitulino wrote:
> QInt is a high-level data type that can be used to store integers
> and perform type-safe conversions.
>
> The following functions are available:
>
> - qint_from_int()   Create a new QInt from an int
> - qint_from_int64() Create a new QInt from an int64_t
> - qint_to_int()     Export QInt to int
> - qint_to_uint64()  Export QInt to uint64_t
> - qint_to_uint32()  Export QInt to uint32_t
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   Makefile  |    1 +
>   qint.c    |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qint.h    |   20 +++++++++++++

I don't see the need for a separate header file since 99.99% of the 
QObject clients would include all the .h files; also, most function 
could be inline in qint.h (or qobject.h) given that struct QInt is not 
opaque.

I also noticed this from 01/29 only now:

+#define QType_HEAD  \
+    QObject base;

remove the trailing semicolon to ease automatic indentation in editors.

Paolo
Paolo Bonzini - Aug. 20, 2009, 9:22 a.m.
On 08/20/2009 09:51 AM, Avi Kivity wrote:
> On 08/20/2009 02:07 AM, Luiz Capitulino wrote:
>> QInt is a high-level data type that can be used to store integers
>> and perform type-safe conversions.
>>
>> The following functions are available:
>>
>> - qint_from_int() Create a new QInt from an int
>> - qint_from_int64() Create a new QInt from an int64_t
>> - qint_to_int() Export QInt to int
>> - qint_to_uint64() Export QInt to uint64_t
>> - qint_to_uint32() Export QInt to uint32_t
>
> Why aren't the conversion functions symmetrical?

I guess because an uint32_t would extend directly to uint64_t.  But, the 
solution would be to drop qint_from_int (since also an int32_t extends 
correctly to uint64_t [1]) and rename qint_from_int64:

Paolo

[1] example program to avoid getting into C standardese:

#include <stdint.h>

int f(uint64_t i)
{
   printf ("%lld\n", i);
}

int main()
{
   int32_t i = -1<<31;
   uint32_t j = 1 << 31;
   f(i);
   f(j);
}
Luiz Capitulino - Aug. 20, 2009, 1:24 p.m.
On Thu, 20 Aug 2009 10:51:48 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/20/2009 02:07 AM, Luiz Capitulino wrote:
> > QInt is a high-level data type that can be used to store integers
> > and perform type-safe conversions.
> >
> > The following functions are available:
> >
> > - qint_from_int()   Create a new QInt from an int
> > - qint_from_int64() Create a new QInt from an int64_t
> > - qint_to_int()     Export QInt to int
> > - qint_to_uint64()  Export QInt to uint64_t
> > - qint_to_uint32()  Export QInt to uint32_t
> >    
> 
> Why aren't the conversion functions symmetrical?

 Are you referring to all of them?

 Thinking about this now, qint_from_int() is not needed,
I can drop it and rename qint_from_int64().

 Regarding the 'export' ones, I thought that the compiler
would warn about something like:

	int index = qint_to_int64(qi);

 forcing the user to cast by hand.

 But testing it now, it doesn't happen. :)
Avi Kivity - Aug. 20, 2009, 1:26 p.m.
On 08/20/2009 04:24 PM, Luiz Capitulino wrote:
> On Thu, 20 Aug 2009 10:51:48 +0300
> Avi Kivity<avi@redhat.com>  wrote:
>
>    
>> On 08/20/2009 02:07 AM, Luiz Capitulino wrote:
>>      
>>> QInt is a high-level data type that can be used to store integers
>>> and perform type-safe conversions.
>>>
>>> The following functions are available:
>>>
>>> - qint_from_int()   Create a new QInt from an int
>>> - qint_from_int64() Create a new QInt from an int64_t
>>> - qint_to_int()     Export QInt to int
>>> - qint_to_uint64()  Export QInt to uint64_t
>>> - qint_to_uint32()  Export QInt to uint32_t
>>>
>>>        
>> Why aren't the conversion functions symmetrical?
>>      
>   Are you referring to all of them?
>    

You're converting from signed ints, but back into unsigned ints.
Luiz Capitulino - Aug. 20, 2009, 1:44 p.m.
On Thu, 20 Aug 2009 10:37:16 +0200
Paolo Bonzini <bonzini@gnu.org> wrote:

> On 08/20/2009 01:07 AM, Luiz Capitulino wrote:
> > QInt is a high-level data type that can be used to store integers
> > and perform type-safe conversions.
> >
> > The following functions are available:
> >
> > - qint_from_int()   Create a new QInt from an int
> > - qint_from_int64() Create a new QInt from an int64_t
> > - qint_to_int()     Export QInt to int
> > - qint_to_uint64()  Export QInt to uint64_t
> > - qint_to_uint32()  Export QInt to uint32_t
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> >   Makefile  |    1 +
> >   qint.c    |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   qint.h    |   20 +++++++++++++
> 
> I don't see the need for a separate header file since 99.99% of the 
> QObject clients would include all the .h files; also, most function 
> could be inline in qint.h (or qobject.h) given that struct QInt is not 
> opaque.

 We could have them in qemu-common.h or create a qemu-objects.h, but
I would dislike merging everything in only one header file: different
things in different header files is good practice.

 With regard to inline you are right, but I avoided it as there's always
a debate on what should be inlined and what shouldn't, I really don't
want to get into this.

> I also noticed this from 01/29 only now:
> 
> +#define QType_HEAD  \
> +    QObject base;
> 
> remove the trailing semicolon to ease automatic indentation in editors.

 OK
Luiz Capitulino - Aug. 20, 2009, 2:51 p.m.
On Thu, 20 Aug 2009 16:26:33 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/20/2009 04:24 PM, Luiz Capitulino wrote:
> > On Thu, 20 Aug 2009 10:51:48 +0300
> > Avi Kivity<avi@redhat.com>  wrote:
> >
> >    
> >> On 08/20/2009 02:07 AM, Luiz Capitulino wrote:
> >>      
> >>> QInt is a high-level data type that can be used to store integers
> >>> and perform type-safe conversions.
> >>>
> >>> The following functions are available:
> >>>
> >>> - qint_from_int()   Create a new QInt from an int
> >>> - qint_from_int64() Create a new QInt from an int64_t
> >>> - qint_to_int()     Export QInt to int
> >>> - qint_to_uint64()  Export QInt to uint64_t
> >>> - qint_to_uint32()  Export QInt to uint32_t
> >>>
> >>>        
> >> Why aren't the conversion functions symmetrical?
> >>      
> >   Are you referring to all of them?
> >    
> 
> You're converting from signed ints, but back into unsigned ints.

 Sometimes you are so brief that a range of problems come to my
mind. :)

 Basically, I'm doing what the code I'm replacing does: the top
level function (get_expr()) converts what was typed by the
user to int64_t. Command handlers, in turn, use this in several
ways: some want int, some uint32_t, some uint64_t.

 As I said, I think we can live only with:

- qint_from_int(int64_t)
- qint_to_int64(const Qint *)
Avi Kivity - Aug. 20, 2009, 2:55 p.m.
On 08/20/2009 05:51 PM, Luiz Capitulino wrote:
>   Sometimes you are so brief that a range of problems come to my
> mind. :)
>    

Sorry about that.  But if you find problems I hadn't thought of, it's a 
good thing :)

>   Basically, I'm doing what the code I'm replacing does: the top
> level function (get_expr()) converts what was typed by the
> user to int64_t. Command handlers, in turn, use this in several
> ways: some want int, some uint32_t, some uint64_t.
>
>   As I said, I think we can live only with:
>
> - qint_from_int(int64_t)
> - qint_to_int64(const Qint *)
>    

Yeah.
Luiz Capitulino - Aug. 20, 2009, 3:14 p.m.
On Thu, 20 Aug 2009 17:55:23 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/20/2009 05:51 PM, Luiz Capitulino wrote:
> >   Sometimes you are so brief that a range of problems come to my
> > mind. :)
> >    
> 
> Sorry about that.  But if you find problems I hadn't thought of, it's a 
> good thing :)

 Sure, I forgot to add this is the good side.

Patch

diff --git a/Makefile b/Makefile
index f6bdf84..d604302 100644
--- a/Makefile
+++ b/Makefile
@@ -90,6 +90,7 @@  obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o
 obj-y += qemu-char.o aio.o net-checksum.o savevm.o
 obj-y += msmouse.o ps2.o
 obj-y += qdev.o qdev-properties.o ssi.o
+obj-y += qint.o
 
 obj-$(CONFIG_BRLAPI) += baum.o
 obj-$(CONFIG_WIN32) += tap-win32.o
diff --git a/qint.c b/qint.c
new file mode 100644
index 0000000..8398824
--- /dev/null
+++ b/qint.c
@@ -0,0 +1,92 @@ 
+/*
+ * QInt data type.
+ *
+ * Copyright (C) 2009 Red Hat Inc.
+ *
+ * Authors:
+ *  Luiz Capitulino <lcapitulino@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+#include "qint.h"
+#include "qobject.h"
+#include "qemu-common.h"
+
+static const QType qint_type;
+
+/**
+ * qint_from_int64(): Create a new QInt from an int64_t
+ *
+ * Return new reference.
+ */
+QInt *qint_from_int64(int64_t value)
+{
+    QInt *qi;
+
+    qi = qemu_malloc(sizeof(*qi));
+    qi->number = value;
+    QOBJECT_INIT(qi, &qint_type);
+
+    return qi;
+}
+
+/**
+ * qint_from_int(): Create a new QInt from an int
+ *
+ * Return new reference.
+ */
+QInt *qint_from_int(int value)
+{
+    return qint_from_int64(value);
+}
+
+/**
+ * qint_to_int(): Export QInt to int type
+ */
+int qint_to_int(const QInt *qi)
+{
+    return qi->number;
+}
+
+/**
+ * qint_to_uint32(): Export QInt to uint32_t type
+ */
+uint32_t qint_to_uint32(const QInt *qi)
+{
+    return qi->number;
+}
+
+/**
+ * qint_to_uint64(): Export QInt to uint64_t type
+ */
+uint64_t qint_to_uint64(const QInt *qi)
+{
+    return qi->number;
+}
+
+/**
+ * qobject_to_qint(): Convert a QObject into a QInt
+ */
+QInt *qobject_to_qint(const QObject *obj)
+{
+    if (qobject_type(obj) != QTYPE_QINT)
+        return NULL;
+
+    return container_of(obj, QInt, base);
+}
+
+/**
+ * qint_destroy_obj(): Free all memory allocated by a
+ * QInt object
+ */
+static void qint_destroy_obj(QObject *obj)
+{
+    assert(obj != NULL);
+    qemu_free(qobject_to_qint(obj));
+}
+
+static const QType qint_type = {
+    .code = QTYPE_QINT,
+    .destroy = qint_destroy_obj,
+};
diff --git a/qint.h b/qint.h
new file mode 100644
index 0000000..1307761
--- /dev/null
+++ b/qint.h
@@ -0,0 +1,20 @@ 
+#ifndef QINT_H
+#define QINT_H
+
+#include "qobject.h"
+#include "qemu-common.h"
+#include <stdint.h>
+
+typedef struct QInt {
+    QType_HEAD
+    int64_t number;
+} QInt;
+
+QInt *qint_from_int(int value);
+QInt *qint_from_int64(int64_t value);
+int qint_to_int(const QInt *qi);
+uint32_t qint_to_uint32(const QInt *qi);
+uint64_t qint_to_uint64(const QInt *qi);
+QInt *qobject_to_qint(const QObject *obj);
+
+#endif /* QINT_H */
diff --git a/qobject.h b/qobject.h
index c23b5f3..8ff7bfd 100644
--- a/qobject.h
+++ b/qobject.h
@@ -41,6 +41,7 @@ 
 
 typedef enum {
     QTYPE_NONE,
+    QTYPE_QINT,
 } qtype_code;
 
 struct QObject;