I prefer item 3 but it comes down to preference.
JonH
2010-02-23 20:50:22
+3
A:
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.
Wookai
2010-02-23 20:51:42
+1 I agree it's a lot easier to read device_address than cmd[2].
Kevin Gale
2010-02-23 20:54:02
#3 would be the easiest to read if instead of using numbers, there were predefined constants for the indexes.
Javier
2010-02-23 20:56:53
Why would 2 do better than 1 at not losing space? In 1, three `unsigned char`s are created, and in 2 three `unsigned char *`s are created.
David Thornley
2010-02-23 20:56:56
@Kevin - but that is only because you know cmd[2] is a device address, so what is the difference? I could of easily just assigned it to a temp variable called device_address to fulfill the wish...My point is the programmer knows that cmd[2] or cmd[index] represents a specific value for a specific type. Otherwise he / she wouldn't know how to call do_stuff() //in this case...
JonH
2010-02-23 20:57:17
@Javier - I agree..
JonH
2010-02-23 20:57:44
@David: I edited my answer ;) ! I was referring to bigger data structures, where a pointer is really smaller than the whole data. But in this case, it does not change much !
Wookai
2010-02-23 21:03:04
@JonH: If you read the code for the first time, how would you know what is cmd[2] in number 3 ? In 1 and 2, you don't need any other knowledge to understand what the code does (provided that do_stuff has a meaningful name), which is IMO better and more readable.
Wookai
2010-02-23 21:03:34
@JonH - Why should anyone have to remember that cmd[2] means something. Either use a variable or name the index. 2 is a magic number in this case and magic numbers are to be avoided.
Kevin Gale
2010-02-23 21:04:30
+2
A:
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.
Max Shawabkeh
2010-02-23 20:53:01
+4
A:
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);
John Knoeller
2010-02-23 20:57:37
*Obligatory struct padding warning*: This, of course, should be done with care. The struct is not guaranteed to have its elements contiguous in memory, so the struct may not match exactly the layout of the input data. If your compiler has a something like gcc's __attribute__((packed)), it would at least be a good idea to use that. This (as it is) may not even port bw differing versions of the same compiler.
Tim Schaeffer
2010-02-23 21:30:13
@Alok: as long as the members of CMD are byte sized, there are no alignment issues.
John Knoeller
2010-02-23 21:30:32
Using a struct overlay like this avoids the data duplication of #1, the extra variables of #1 and #2, and the ambiguity of #3. As an added benefit, it reinforces the idea that these values are related to each other.
bta
2010-02-23 22:32:22
@Alok: the standard doesn't go into that sort of detail, saying only that insertion of padding is _permitted_ without violating the standard. But compiler writers aren't fools, and the standard isn't the only thing that constrains them.
John Knoeller
2010-02-23 22:46:07
A:
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)
Larry_Croft
2010-02-23 21:11:33
+1
A:
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.
Andrew O'Reilly
2010-02-23 22:11:38
A:
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.
Liz Albin
2010-02-23 22:31:20