On Tue, 1 May 2012 20:30:15 +0100 Daniel Stone <[email protected]> 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 <[email protected]> > --- > 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
