Code review comment for lp:~midauth-pisa-devs/hipl/midauth-firewall

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