On Sat, Oct 22, 2011 at 02:44:31PM +0200, Christof Mroz wrote: > On 22.10.2011 13:36, Diego Biurrun wrote: > >>>>+static void *rebase(const void *const field, > >>>>+ const void *const old, > >>>>+ void *const new) > >>>>+{ > >>>>+ return (char *) new + ((const char *) field - (const char *) old); > >>> > >>>pointless parentheses > >> > >>(field - old) is usually a low "number", if we treat pointers > >>numerically, while (new + field) is usually big. Could even be > >>bigger than the machine pointer size, especially considering the > >>gamut of devices we support. Are you sure the compiler will do the > >>right thing here if we drop the parentheses? > > > >I do not believe that parentheses work in the way you hope for here. > >I would assume that the compiler is free to evaluate the operations > >in arbitrary order. Have you verified that the behavior you seem to > >be relying on is actually implemented like you expect? > > Good question. Admittedly I didn't know about the details so far > either, so I dug around a bit and Example 6 in section 5.1.2.3 of > [1] (page 15) seems to answer our question. It states: > >On a machine in which overflow silently generates some value and where > >positive and negative overflows cancel, the above expression statement > >can be rewritten by the implementation in any of the above ways > >because the same result will occur. > This is the case on x86 for example, meaning that we could safely > overflow around as long as we don't care about the bits outside our > machine word, on this platform. I can show an example if you don't > believe me. > > On the other hand, it is stated that (in reference to an example > involving addition/subtraction): > >On a machine in which overflows produce an explicit trap and in which > >the range of values representable by an int is [32768, +32767], the > >implementation cannot rewrite this expression as [...] > So by keeping the parentheses, we can ensure that our code runs on > this kind of platform too, right? > > [1] http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf The standard is quoted - we're taking this to a whole new level :) I would suggest a comment to explain the parentheses in this case. Very few people working on HIPL will be aware of why they might be needed. > >>>>+ while ((current = hip_get_next_param_readwrite(hip, current))) { > >>>>+ if (hip_get_param_type(current)>= param_type) { > >>>>+ uint8_t *const splice = (uint8_t *const) current; > >>>>+ > >>>>+ memmove(splice + param_len, splice, end - splice); > >>>>+ out = splice; > >>>>+ break; > >>> > >>>The splice variable looks unnecessary. > >> > >>Personally, I found the number of casts distracting. > > > >If I did not misread the code, then no casts should be necessary. > > It's true that memmove() accepts void*, but current must be > re-interpreted as a uint8_t buffer for the pointer arithmetic to > work correctly (since param_len is given as a byte count). Whatever you prefer then. Diego