Binary reader and writer

I recently implemented a binary reader/writer for my multiplayer game.

Now reading bytes looks like this:

static int32_t readInt32(std::vector& msg, int *off)
{
    if ((*off) > msg.size()) return 0;
    int32_t result; int size = sizeof (result);
    memcpy(&result, msg.data() + (*off), size); (*off) += size; 
    return result;
}

And writing bytes looks like this:

static void writeInt32(std::vector* msg, int32_t value)
{
    uint8_t const * array = reinterpret_cast(&value);

    for (std::size_t i = 0; i != sizeof(value); ++i)
    {
        (*msg).push_back(array[i]);
    }
}

How can I improve this?

I see a number of things that could help you improve your program.

Fix your formatting

Crowding multiple statements on a single line makes your program harder to read and understand. So instead of this:

if ((*off) > msg.size()) return 0;
int32_t result; int size = sizeof (result);

Write this:

if ((*off) > msg.size()) {
    return 0;
}
int32_t result; 
int size = sizeof (result);

Use the required #includes

The code uses std::vector which means that it should #include . It was not difficult to infer, but it helps reviewers if the code is complete. For this code, it appears that you need these:

#include 
#include 
#include 

Use C++-style includes

Instead of including string.h you should instead use #include . The difference is in namespaces as you can read about in this question. I infer that the code used that because the call to memcpy was not std::memcpy.

Prefer passing a reference to a raw pointer

The writeInt32() function takes a pointer to a vector, but what if the pointer is nullptr? Clearly the function will fail, and probably crash the program if that happens. Better, then, would be to pass a reference instead, which says that it really MUST be a std::vector and nothing else.

Avoid portability problems

The endianness of computers is not fixed by the C++ language standard, so your memory copy or memory casting is not going to work the same way on all machines. This portability problem can be solved by doing one of two things: either rely only on guarantees that are actually made by the standard or use conversion routines to put everything in the same order. This often comes up in network communications, so some platforms have htonl() and friends to convert from "host order" which might be big-endian or little-endian, to network order which is always big-endian. Note, however, that this has its own portability issues because those functions are not part of the C++ standard. So the other way to do it is to do it yourself.

static void writeInt32(std::vector& msg, std::int32_t value)
{
    for (auto i = sizeof(value); i; --i, value >>= 8) {
        msg.push_back(value & 0xff);
    }
}

This is shorter and also portable. In this version, i is declared to be auto but declaring it to be of type std::size_t, as you had it, is another way to do it and even works if you're not using a C++11 compiler.

Be careful with signed and unsigned

In the readInt32 routine, the code compares an int to a size_t, but size_t is unsigned and int is signed. In this case, your off variable should probably be declared as being of std::size_t * type.

Don't fail silently

In the readInt32 function, a returned value of 0 might mean that a value of zero was actually read or it might mean that the offset pointer was off the end of the message. Unfortunately with the current code there's no way to differentiate these cases. I'd suggest throwing an exception instead.

Be careful with memcpy

The code currently catches the case in which the offset is beyond the end of the vector, but what if it's pointing instead to the last byte of the vector? In this case, memcpy will copy that last byte and whatever happens to be in memory beyond that. This is not likely to be a useful value. In this case, I'd recommend not using memcpy at all and rewriting the code to be portable as mentioned above.

Reconsider the interface

Right now, the code consists of two functions which deal with writing/reading a std::int32_t value to/from a message. If your code frequently does things with such messages, it might be better to create a Message class and have member functions that read and write particular types of values, such as your std::int32_t values used here. I'd also recommend that raw pointers such as off not be part of the interface at all.

StackExchange Code Review Q#139823, answer score: 3