On Wed, May 2, 2012 at 11:48 AM, Daniel Stone <[email protected]> wrote: > Hi, > > On 2 May 2012 12:57, Pekka Paalanen <[email protected]> wrote: >> Hmm, any reason for not doing this instead? >> *p = (int32_t)trunc(d * 256.0) > > Wow is that ever embarrassing. > >> 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? > > Thanks for the review. I've fixed this now to use trunc(foo * 256.0) > or / 256.0 as appropriate, plus fixed up the sign issues. There are > some tests in there now for both negative and large numbers and they > seem to work fine. > >> 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? > > I'm not sure about the typedef, but I've at least put range handling > into the marshalling code so it rejects too-large numbers. Kristian?
I wasn't thinking that we'd convert to doubles at all. We could just make it be an int32_t or perhaps a wl_fixed_t typedef. But the point is that the canonical type of input coordinates is 24.8 fixed point, not just in the protocol but also in the API. There may be implementation limitations on both sides (weston using GLfloats, toolkits rounding to integers, for example), but those are implementation decisions, not something we want to encode in the library API. A wl_fixed_t is probably the nicest way to go, and I think that we should add support for it in the protocol xml. This lets us generate the right types in the stubs and the debug code can convert it to double as it prints it, but on the wire and in marshalling it's just a int32_t. Kristian _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
