On Sun, 11 Jan 2004, Matthew Dharm wrote:
> On Mon, Jan 12, 2004 at 02:44:10AM +0100, [EMAIL PROTECTED] wrote:
> > Hmm. I am not precisely happy with improvements like
> >
> > - ptr[4] = MSB_of(info->pagesize>>16);
> > - ptr[5] = LSB_of(info->pagesize>>16);
> > - ptr[6] = MSB_of(info->pagesize&0xFFFF);
> > - ptr[7] = LSB_of(info->pagesize&0xFFFF);
> > + ((u32 *) ptr)[1] = cpu_to_be32(info->pagesize);
> >
> > A good compiler generates the same code, and for a human
> > like me the above four statements show much more clearly
> > that values are assigned to ptr[4..7] than the ugly cast below.
>
> Actually, I think the new way is more readable. And, apparently, so does
> Alan. Considering he contributes an amazing amount of code on a regular
> basis, I'm inclined to go with his way.
My opinion is slightly mixed. I definitely think the old way is too
repetitive, unclear, and error-prone. On the other hand, I also dislike
the ugly cast. A possible alternative, also ugly but at least indicating
more clearly which elements are affected, would be
*((u32 *) &ptr[4]) = cpu_to_be32(info->pagesize);
To me it makes no difference whether this version or the one in the patch
gets used.
Actually, I think the best solution would be something like this:
#include <asm/unaligned.h>
static void inline put_be32(void *dest, u32 val)
{
put_unaligned(cpu_to_be32(val), (u32 *) dest);
}
put_be32(&ptr[4], info->pagesize);
(Maybe some people would prefer to have the arguments of put_be32 go in
the opposite order.)
This is much clearer than any of the alternatives and it can be used for
unaligned references, which the assignments above cannot. Luckily we
know in this case that ptr[4] will have the necessary alignment; there
were other examples in sddr09.c where that wasn't true. Unfortunately
there doesn't seem to be anything like put_be32 available in the kernel,
which means we would be forced to roll our own definition as above.
Alan Stern
-------------------------------------------------------
This SF.net email is sponsored by: Perforce Software.
Perforce is the Fast Software Configuration Management System offering
advanced branching capabilities and atomic changes on 50+ platforms.
Free Eval! http://www.perforce.com/perforce/loadprog.html
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
