Good Form: Pointer vs. local variable vs. Array Index

Forgive me if this comes across as a trivial question -开发者_运维技巧 I'm usually a control systems guy (plc's and automation) but have found myself involved in some embedded micro-controller and PC projects lately.

Let's say I have a function that accepts a pointer to an array of 'command bytes', typically 5 or 10 bytes in length, like so:

char cmd_i2c_read(unsigned char *cmd, unsigned short cmd_len) { ... }

I want to decode the command bytes (*cmd).

Is it better form to:

1. Create local variables indicating the purpose of each byte:

   unsigned char device_address = cmd[2];
   unsigned char register_address = cmd[3];
   unsigned char num_bytes = cmd[4];
   
   // use the local variables:
   if(num_bytes &le 0xFF) {
       do_stuff(device_address, register_address, num_bytes);
   }

2. Create local pointers:

   unsigned char *device_address = &cmd[2];
   unsigned char *register_address = &cmd[3];
   unsigned char *num_bytes = &cmd[4];
   
   // use the pointers:
   if(*num_bytes &le 0xFF) {
       do_stuff(*device_address, *register_address, *num_bytes);
   }

3. Index the *cmd array directly:

   if(cmd[4] <= 0xFF) { do_stuff(cmd[2], cmd[3], cmd[4]); }

Option 1 is clear but a bit wordy. I don't like 2 at all, and 3 is hard to understand. Personally I prefer to use structs for this sort of thing.

typedef struct  {
   unsigned char whatever[2];
   unsigned char device_address;
   unsigned char register_address;
   unsigned char num_bytes;
   }  CMD;

CMD * pcmd = (CMD *)&cmd[0];

// use the local variables:
if(num_bytes ≤ 0xFF) {
    do_stuff(pcmd->device_address, pcmd->register_address, pcmd->num_bytes);

I prefer item 3 but it comes down to preference.

IMHO, the first way is better. It is much easier to read than number 3, because you don't need to know the signature of the function to understand what the parameters are.

For bigger data structures, I would go with number 2, i.e. use pointer such that you don't have to copy the values. But in this case, the difference is not significant, and I think that the */& slightly reduce the readability.

Definitely #1. It makes the rest of the code much more readable, and using a pointer where a normal variable will do complicates the matter for no reason. The compiler will optimize away whatever microscopic performance gains you might get from #3.

If it were me I'd write this as #3 but with symbolic names for the array indices to improve readability:-

#define DEVICE_ADDRESS 2
#define REGISTER_ADDRESS 3
#define NUM_BYTES 4

if (cmd[NUM_BYTES] <= 0xFF) {
    do_stuff(cmd[DEVICE_ADDRESS], cmd[REGISTER_ADDRESS], cmd[NUM_BYTES]);
}

You can of course replace the macros with const ints, enums, etc. The reason why I like this option is that the other two options require the use of additional local variables. The compiler may or may not optimize these away depending on its implementation and your chosen level of optimization but they just seem like an unnecessary level of extra indirection to me.

I like 1 the most, it's much more readable than the others.

If performance is an important issue (i mean important like in "we need every 0.01% speedup we can get"), you will have to benchmark cause it really depends on the compiler which sequence will end up in the fastest code (1 may have unnecessary copys, 2 may contain superflous loads due to pointer aliasing restrictions, 3 may lead to superflous loads if the register allocator is really messed up)

To me it depends on What You'll Be Doing with your buffer,

All things being equal though, I'd probably prefer to have the buffer be a struct (assume alignment warings) and, failing that, to directly index the buffer, although not with magic numbers.