[PATCH wayland 4/5] Add support for signed 24.8 decimal numbers
Pekka Paalanen
ppaalanen at gmail.com
Wed May 2 04:57:20 PDT 2012
On Tue, 1 May 2012 20:30:15 +0100
Daniel Stone <daniel at fooishbar.org> wrote:
> signed_24_8 is a signed decimal type which offers a sign bit, 23 bits of
> integer precision, and 8 bits of decimal precision. This is converted
> to double on the C API side, as passing through varargs involves an
> implicit type promotion to double anyway.
>
> Signed-off-by: Daniel Stone <daniel at fooishbar.org>
> ---
> src/Makefile.am | 4 ++--
> src/connection.c | 29 +++++++++++++++++++++++++++++
> src/scanner.c | 9 +++++++++
> tests/connection-test.c | 20 ++++++++++++++++++++
> 4 files changed, 60 insertions(+), 2 deletions(-)
>
...
> diff --git a/src/connection.c b/src/connection.c
> index 06cc66f..15d571d 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -22,6 +22,7 @@
>
> #define _GNU_SOURCE
>
> +#include <math.h>
> #include <stdlib.h>
> #include <stdint.h>
> #include <string.h>
> @@ -382,6 +383,8 @@ wl_message_size_extra(const struct wl_message *message)
> case 'h':
> extra += sizeof (int);
> break;
> + case 'f':
> + extra += sizeof (double);
> default:
> break;
> }
> @@ -415,6 +418,8 @@ wl_closure_vmarshal(struct wl_closure *closure,
> const char **sp, *s;
> char *extra;
> int i, count, fd, extra_size, *fd_ptr;
> + double d;
> + uint32_t u;
>
> extra_size = wl_message_size_extra(message);
> count = strlen(message->signature) + 2;
> @@ -428,6 +433,17 @@ wl_closure_vmarshal(struct wl_closure *closure,
>
> for (i = 2; i < count; i++) {
> switch (message->signature[i - 2]) {
> + case 'f':
> + closure->types[i] = &ffi_type_double;
> + closure->args[i] = p;
> + if (end - p < 1)
> + goto err;
> + d = va_arg(ap, double);
> + u = trunc(d);
> + *p = u << 8;
> + *p |= (uint32_t) (trunc((fabs(d - u)) * (1 << 8))) & 0xff;
Hmm, any reason for not doing this instead?
*p = (int32_t)trunc(d * 256.0)
Also, any rationale in choosing trunc() instead of round() or
ceil/floor?
trunc() makes
0.9 -> 0
-0.9 -> 0
which means the length of "zero" is double the length of any other
number. No?
Since we use double as the API type, should there not be a check for
out-of-bounds values? Perhaps even #defined min and max values apps
could use for their own additional checks. And that raises the
question, should we have a
typedef double signed_24_8;
which would seem odd at first, but make it apparent on the API
that you cannot pass just whatever doubles there?
> + p++;
> + break;
> case 'u':
> closure->types[i] = &ffi_type_uint32;
> closure->args[i] = p;
> @@ -567,6 +583,8 @@ wl_connection_demarshal(struct wl_connection *connection,
> unsigned int i, count, extra_space;
> struct wl_object **object;
> struct wl_array **array;
> + int32_t si;
> + double *d;
>
> count = strlen(message->signature) + 2;
> if (count > ARRAY_LENGTH(closure->types)) {
> @@ -611,6 +629,15 @@ wl_connection_demarshal(struct wl_connection *connection,
> closure->types[i] = &ffi_type_sint32;
> closure->args[i] = p++;
> break;
> + case 'f':
> + closure->types[i] = &ffi_type_double;
> + si = *p;
> + p++;
> + d = (double *) extra;
> + extra += sizeof *d;
> + closure->args[i] = d;
> + *d = (si >> 8) + (((double) (si & 0xff)) / (1 << 8));
Couldn't this be simplified as:
*d = (double)si / 256.0;
> + break;
> case 's':
> closure->types[i] = &ffi_type_pointer;
> length = *p++;
> @@ -837,6 +864,8 @@ wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
> case 'i':
> fprintf(stderr, "%d", value->uint32);
> break;
> + case 'f':
> + fprintf(stderr, "%f", (float) ((value->uint32 >> 8) + ((value->uint32 & 0xff) >> 8)));
(value->uint32 >> 8) + ( (value->uint32 & 0xff) >> 8 )
integer part, ok ^
but the latter part makes no sense to me. Why is this code different to
the one quoted above? Why unsigned?
> case 's':
> fprintf(stderr, "\"%s\"", value->string);
> break;
...
> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 54ac423..5f090b7 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -20,6 +20,7 @@
> * OF THIS SOFTWARE.
> */
>
> +#include <math.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdarg.h>
> @@ -153,6 +154,7 @@ struct marshal_data {
> int32_t i;
> const char *s;
> int h;
> + double d;
> } value;
> };
>
> @@ -288,6 +290,13 @@ validate_demarshal_h(struct marshal_data *data,
> }
>
> static void
> +validate_demarshal_d(struct marshal_data *data,
> + struct wl_object *object, double d)
> +{
> + assert(data->value.d >= (d - 0.0025) && data->value.d <= (d + 0.0025));
At least for me this would be easier to read:
assert(fabs(data->value.d - d) <= 1.0 / 256.0);
> +}
> +
> +static void
> demarshal(struct marshal_data *data, const char *format,
> uint32_t *msg, void (*func)(void))
> {
> @@ -335,6 +344,13 @@ TEST(connection_demarshal)
> memcpy(&msg[3], data.value.s, msg[2]);
> demarshal(&data, "s", msg, (void *) validate_demarshal_s);
>
> + data.value.d = 5678.1234;
> + msg[0] = 400200;
> + msg[1] = 12;
> + msg[2] = (uint32_t) (trunc(data.value.d)) << 8;
> + msg[2] |= (uint32_t) (trunc((fabs(data.value.d - (msg[2] >> 8))) * (1 << 8))) & 0xff;
That test code differs from the original, quote:
> + u = trunc(d);
> + *p = u << 8;
> + *p |= (uint32_t) (trunc((fabs(d - u)) * (1 << 8))) & 0xff;
Isn't the test code zeroing the highest 8 bits, where you were supposed
to use 'u'?
> + demarshal(&data, "f", msg, (void *) validate_demarshal_d);
> +
> release_marshal_data(&data);
> }
>
> @@ -400,6 +416,10 @@ TEST(connection_marshal_demarshal)
> marshal_demarshal(&data, (void *) validate_demarshal_h,
> 8, "h", data.value.h);
>
> + data.value.d = 1234.5678;
> + marshal_demarshal(&data, (void *) validate_demarshal_d,
> + 12, "f", data.value.d);
Could you also add marshal_demarshal tests for negative, +/- huge, and
near-zero values?
> +
> release_marshal_data(&data);
> }
>
Thanks,
pq
More information about the wayland-devel
mailing list