int valueToWrite = 0xFFFFFFFF;
static char buffer2[256];
int* writePosition = (int* ) &buffer2[5];
*writePosition = valueToWrite;
//OR
* ((int*) &buffer2[10] ) = valueToWrite;
Now, I ask you guys which one do you find more readable. The 2 step technique involving a temporary variable or the one step technique?
Do not worry about optimization, they both optimize to the same thing, as you can see here. Just tell me which one is more readable for you.
or DWORD PTR ?buffer2@?1??main@@9@4PADA+5, -1
or DWORD PTR ?buffer2@?1??main@@9@4PADA+10, -1
int* writePosition = (int* ) &buffer2[5]
Or
*((int*) &buffer2[10] ) = valueToWrite;
Are both incorrect because on some platforms access to unaligned values (+5 +10) may cost hundreds of CPU cycles and on some (like older ARM) it would cause an illegal operation.
The correct way is:
memcpy( buffer+5, &valueToWrite, sizeof(valueToWrite));
And it is more readable.
Once you encapsulate it inside a class, it does not really matter which technique you use. The method name will provide the description as to what the code is doing. Thus, in most cases you will not have to delve into the actual impl. to see what is going on.
class Buffer
{
char buffer2[256];
public:
void write(int pos, int value) {
int* writePosition = (int*) &buffer2[pos];
*writePosition = value;
}
}
If I was forced to choose, I'd say 1. However, I'll note the code as presented is very C like either way; I'd shy away from either and re-examine the the problem. Here's a simple one that is more C++-y
const char * begin = static_cast<char*>(static_cast<void*>(&valueToWrite));
std::copy(begin, begin+sizeof(int), &buffer2[5]);
The first example is more readable purely on the basis that your brain doesn't have to decipher the pointer operations globed together.
This will reduce the time a developer looking at the code for the first time needs to understand what's actually going. In my experience this loosely correlates to reducing the probability of introducing new bugs.
I find the second, shorter one easier to read.
I suspect, however, that this rather depends on whether you are the type of person that can easily 'get' pointers.
The type casting from char* to int* is a little awkward, though. I presume there is a good reason this needs to be done.
Watch out -- this code probably won't work due to alignment issues! Why not just use memset
?
#include <string.h>
memset(buffer2+10, 0xFF, 4);
If you can afford to tie yourself to a single compiler (or do preprocessor hacks around compatiblity issues), you can use a packed-structs option to get symbolic names for the values you're writing. For example, on GCC:
struct __attribute__ ((__packed__)) packed_struct
{
char stuff_before[5]
int some_value;
}
/* .... */
static char buffer2[256];
struct packed_struct *ps = buffer2;
ps->some_value = valueToWrite;
This has a number of advantages:
But again, has the major disadvantage of not having any standardized syntax.
Most readable would be either variant with a comment added on what you're doing there.
That being said, I despise variables introduced simply for the purpose of a one-time use a couple of lines later. Doing most of my work in the maintenance area, getting dozens of variable names pushed in my face that are poor efforts not having to write an explanatory comment sets me on edge.
Definitely:
* ((int*) &buffer2[10] ) = valueToWrite;
I parse it not in one but few steps, and that is why it is more readable: I have all steps in one line.
From the readability perspective, the behaviour of your code should be clear, but "clear" is not how I would describe either of these alternatives. In fact, they are the opposite of "clear", as they are non-portable.
On top of alignment issues, there's integer representation (the size varies from system to system, as does sign representation, endianness and padding to throw into the soup). Thus, the behaviour of your code from system to system is erratic.
If you want to be clear about what your algorithm is supposed to do, you should explicitly put each byte into its correct place. For example:
void serialise_uint_lsb(unsigned char *destination, unsigned source) {
destination[0] = source & 0xff; source >>= 8;
destination[1] = source & 0xff; source >>= 8;
assert(source == 0);
}
void deserialise_uint_lsb(unsigned *destination, unsigned char *source) {
*destination = 0;
*destination <<= 8; *destination += source[1];
*destination <<= 8; *destination += source[0];
}
Serialisation and deserialisation are idiomatic concepts for programmers... *printf
and *scanf
are forms of serialisation/deserialisation, for example, except it's idiomatically instilled into your head that the most significant (decimal) digit goes first... which is the problem with your code; your code doesn't tell your system the direction of the integer, how many bytes there are, etc... bad news.
Use a serialisation/deserialisation function. Programmers will understand that best.
User contributions licensed under CC BY-SA 3.0