0

I have a class with a std::vector<unsigned char> mPacket as a packet buffer (for sending UDP strings). There is a corresponding member variable mPacketNumber that keeps track of how many packets have been sent so far.

The first thing I do in the class is reserve space:

mPacket.reserve(400);

and then later, in a loop that runs while I want packets to get sent:

mPacket.clear(); //empty out the vector
long packetLength = 0; //keep track of packetLength for sending udp strings

memcpy(&mPacket[0], &&mPacketNumber, 4); //4 bytes because it's a long
packetLength += 4; //add 4 bytes to the packet length

memcpy(&mPacket[packetLength], &data, dataLength);
packetLength += dataLength;

udp.send(mPacket.data(), packetLength);

Except I realized that nothing was getting sent! How peculiar.

So I dug a bit deeper, and found that mPacket.size() returns zero, while packetLength returns the size I think the packet should be.

I can't think of a reason for mPacket to have zero length -- even if I'm mishandling the data, the header with mPacketNumber should have been written just fine.

Can anyone suggest why I'm running into this problem?

thanks!

nathan lachenmyer
  • 5,298
  • 8
  • 36
  • 57
  • 6
    `reserve` is not the same as `resize`. – Captain Obvlious Jan 05 '15 at 02:03
  • Oh, hmm. So `reserve` reserves memory for me, but without actually calling `push_back`, the vector doesn't 'know' that is has this data pushed to it? Interesting. So my options would be to `resize` the vector so that it knows it can read the adjacent data in memory? – nathan lachenmyer Jan 05 '15 at 03:48
  • What were you doing that you thought would cause the packet not to have zero length? – David Schwartz Jan 05 '15 at 03:51
  • Well, I thought that copying the bytes physically into the memory reserved for the vector would do so. Obviously, I was wrong (because I don't understand exactly how vector works). No need to be snide about it. – nathan lachenmyer Jan 05 '15 at 04:07
  • @nathanlachenmyer The distinction between "reserving the memory" and "creating the elements" is here to support non-trivial types, with constructors or copy constructors or destructors. While you don't use such type here, the interface still needs to respect it for every type. – milleniumbug Jan 05 '15 at 04:12
  • Thanks. I assume that similarly, `std::vector.data()` also doesn't return the right number of elements. – nathan lachenmyer Jan 05 '15 at 21:32
  • @nathanlachenmyer `.data()` returns a pointer to the underlying array. Pointers do not know about the size of the pointed array. That's why you pass the size separately to your `udp.send()` function. – milleniumbug Jan 05 '15 at 21:48

3 Answers3

3

The elements you reserve are not for normal use. The elements are created only if you resize the vector. While it might somehow look it works, it would be a different situation with types having constructors - you could see that the constructors were not called. This is undefined behaviour - you're accessing elements which you aren't allowed in this situation.

The .reserve() operation is normally used together with .push_back() to avoid reallocations, but this is not the case here.

The .size() is not modified if you use .reserve(). You should use .resize() instead.


Alternatively, you can use your copy operation together with .push_back() and .reserve(), but you need to drop the usage of memcpy, and instead use the std::copy together with std::back_inserter, which uses .push_back() to push the elements to the other container:

std::copy(reinterpret_cast<unsigned char*>(&mPacketNumber), reinterpret_cast<unsigned char*>(&mPacketNumber) + sizeof(mPacketNumber), std::back_inserter(mPacket))
std::copy(reinterpret_cast<unsigned char*>(&data), reinterpret_cast<unsigned char*>(&data) + dataLength, std::back_inserter(mPacket));

These reinterpret_casts are vexing, but the code still has one advantage - you won't get buffer overrun in case your estimate was too low.

milleniumbug
  • 15,379
  • 3
  • 47
  • 71
  • Will `std::back_inserter` with reinterpret cast do the right thing in terms of breaking up `long`s into 4 different unsigned chars and pushing them back? – nathan lachenmyer Jan 05 '15 at 21:33
  • @nathanlachenmyer It will do the same thing you are doing right now (as in, copy them sequentially into the target buffer, but with `.push_back`). This code doesn't consider endianness, but so doesn't yours. – milleniumbug Jan 05 '15 at 21:39
  • Thanks! I'll give this a shot; this may be the best solution then. – nathan lachenmyer Jan 05 '15 at 21:49
2

vector, apparently, doesn't count the elements when you call size(). There's a counter variable inside the vector that holds that information, because vector has plenty of memory allocated and can't really know where the end of your data is. It changes counter variable as you add/remove elements using methods of vector object, because they are programmed to do so.

You added data directly to its array pointer, which awakens no reaction of your vector object because it does not use any of its methods. Data is there, but vector doesn't acknowledge it, so counter remains at 0 and size() returns 0.

You should either replace all size() calls with packageLength, or use methods inside your vector to add/remove/read data, or use a dynamically allocated array instead of a vector, or create your own class for containing array and managing it the way you like it. To be honest, using a vector in a situation like this doesn't really make sense.

Vector is a conventional high-level object-oriented component and in most os the cases it should be used that way.

Example of one's own Array class:

If you used your own dynamically allocated array, you'd have to remember its length all the time in order to use it. So lets create a class that will cut us some slack in that. This example has element transfer based on memcpy, and the [] notation works perfectly. It has an original max length, but extends itself when necessary.

Also, this is an in-line class. certain IDEs may ask of you to actually seperate it in header and source file, so you may have to do that yourself.

Add more methods yourself if necessary. When applying this, do not use memcpy unless you're going to change arraySize attribute manually. You've got integrated addFrom and addBytesFrom methods that use memcpy inside (assuming calling array being the destination) and separately increase arraySize. If you do want to use memcpy, setSize method can be used for forcing new array size without modifying the array.

#include <cstring>

//this way you can easily change types during coding in case you change your mind
//more conventional object-oriented method would use templates and generic programming, but lets not complicate too much now
typedef unsigned char type;

class Array  {
private:
    type *array;
    long arraySize;
    long allocAmount; //number of allocated bytes
    long currentMaxSize; //number of allocated elements

    //private call that extends memory taken by the array
    bool reallocMore()
    {
        //preserve old data
        type *temp = new type[currentMaxSize];
        memcpy(temp, array, allocAmount);

        long oldAmount = allocAmount;

        //calculate new max size and number of allocation bytes
        currentMaxSize *= 16;
        allocAmount = currentMaxSize * sizeof(type);

        //reallocate array and copy its elements back into it
        delete[] array;
        array = new type[currentMaxSize];
        memcpy(array, temp, oldAmount);

        //we no longer need temp to take space in out heap
        delete[] temp;

        //check if space was successfully allocated
        if(array) return true;
        else return false;
    }

public:
    //constructor
    Array(bool huge)
    {
        if(huge) currentMaxSize = 1024 * 1024;
        else currentMaxSize = 1024;

        allocAmount = currentMaxSize * sizeof(type);

        array = new type[currentMaxSize];
        arraySize = 0;
    }

    //copy elements from another array and add to this one, updating arraySize
    bool addFrom(void *src, long howMany)
    {
        //predict new array size and extend if larger than currentMaxSize

        long newSize = howMany + arraySize;

        while(true)
        {
            if(newSize > currentMaxSize)
            {
                    bool result = reallocMore();
                    if(!result) return false;
            }
            else break;
        }

        //add new elements

        memcpy(&array[arraySize], src, howMany * sizeof(type));
        arraySize = newSize;

        return true;
    }

    //copy BYTES from another array and add to this one, updating arraySize
    bool addBytesFrom(void *src, long byteNumber)
    {
        //predict new array size and extend if larger than currentMaxSize

        int typeSize = sizeof(type);
        long howMany = byteNumber / typeSize;
        if(byteNumber % typeSize != 0) howMany++;

        long newSize = howMany + arraySize;
        while(true)
        {
            if(newSize > currentMaxSize)
            {
                    bool result = reallocMore();
                    if(!result) return false;
            }
            else break;
        }

        //add new elements

        memcpy(&array[arraySize], src, byteNumber);
        arraySize = newSize;

        return true;
    }

    //clear the array as if it's just been made
    bool clear(bool huge)
    {
        //huge >>> 1MB, not huge >>> 1KB
        if(huge) currentMaxSize = 1024 * 1024;
        else currentMaxSize = 1024;

        allocAmount = currentMaxSize * sizeof(type);

        delete[] array;
        array = new type[currentMaxSize];
        arraySize = 0;
    }

    //if you modify this array out of class, you must manually set the correct size
    bool setSize(long newSize) {
        while(true)
        {
            if(newSize > currentMaxSize)
            {
                    bool result = reallocMore();
                    if(!result) return false;
            }
            else break;
        }

        arraySize = newSize;
    }

    //current number of elements
    long size() {
        return arraySize;
    }

    //current number of elements
    long sizeInBytes() {
        return arraySize * sizeof(type);
    }

    //this enables the usage of [] as in yourArray[i]
    type& operator[](long i)
    {
        return array[i];
    }
};
cruelcore1
  • 578
  • 4
  • 22
  • Fair points! How would you recommend doing it with a dynamically allocated array? – nathan lachenmyer Jan 05 '15 at 21:31
  • well, it's just like every other array, only that sizeof(array) returns only pointer size. I'll write an in-line class in C++ for handling dynamically allocated array more handily and add it to my answer. – cruelcore1 Jan 08 '15 at 17:56
0
mPacket.reserve();
mPacket.resize(4 + dataLength); //call this first and copy into, you can get what you want
mPacket.clear(); //empty out the vector
long packetLength = 0; //keep track of packetLength for sending udp strings

memcpy(&mPacket[0], &&mPacketNumber, 4); //4 bytes because it's a long
packetLength += 4; //add 4 bytes to the packet length

memcpy(&mPacket[packetLength], &data, dataLength);
packetLength += dataLength;

udp.send(mPacket, packetLength);
  • 1
    I don't see any reason to call `reserve` or `clear` since you're calling `resize` and the entire contents will be overwritten. Plus there is no overload of `reserve` that accepts zero arguments. – Captain Obvlious Jan 05 '15 at 03:03
  • std::vector::reserve has not zero argument, that's my bad! but I was remenberd the growth of resize just via push_back, so it have multiple mem alloc, so I call reserve before. – Binghe Zhai Dec 11 '17 at 09:09