1

I'm trying to fill a vector with integers from an array I have but when I check the contents of the vector all the values are zero.

I'm using vector.push_back() to try and fill the vector so it will be in the same order as the array as I needed it ordered in a specific way.

unsigned char* buffer = new unsigned char[size];
std::vector<unsigned char> *data = new std::vector<unsigned char>(size);
fread(buffer, sizeof(unsigned char), size, f);
for(int transfer = 0; transfer < size; transfer += 1){
    std::cout << buffer[transfer];
    data->push_back(buffer[transfer]);
    std::cout << int(data->at(transfer));
}
fclose(f);

When I print the output I can see that the values aren't zero when they're coming from the buffer array but they are when I read from the data vector. Here is some example output φ0$0.

Oblivion
  • 7,176
  • 2
  • 14
  • 33
HoneyBunchers
  • 45
  • 1
  • 7
  • 4
    There is almost never a reason to have a pointer to vector. Where are you learning this stuff from? –  Sep 07 '19 at 19:26
  • Are you learning C++ from a C background? C++ is a complex language but one characteristic of it is that pointers are used much less frequently and it's a good thing too. C++ requires a bit of a mental paradigm shift. Especially to use the std libraries. However, it simplifies a lot of code when written in a C++ style and you will grow to like it. – doug Sep 07 '19 at 19:31
  • @Neil Butterworth Sorry I am just doing c++ as a hobby right now I don't really have a formal source, I set it up like that because I was going to return the pointer later and use the vector for something else. Can I just return the entire vector? – HoneyBunchers Sep 07 '19 at 19:32
  • Yep, returning the vector by value will probably just move it. Using `new` a lot generally just causes problems, because you have lots of things to clean up manually, and it's easy to forgot some, or try to delete the same thing in two places. – Useless Sep 07 '19 at 19:33
  • "I don't really have a formal source" - get a good textbook. –  Sep 07 '19 at 19:33
  • `new std::vector` nope nope nope nope. In modern C++ you never have to use explicit calls to `new`. And in your example, as others have pointed out you don't need pointers at all. – bolov Sep 07 '19 at 19:34
  • @doug yes I took two data structures classes in C but I haven't I taken any classes in C++. I had wondered while I was writing whether I was missing big C++ aspects but I figured I'd find them as I ran into bugs and looked stuff up. – HoneyBunchers Sep 07 '19 at 19:52
  • That's going to be a slow and error-prone way to learn. [The Definitive C++ Book Guide and List](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) – Blastfurnace Sep 07 '19 at 19:55
  • 2
    If you are coming into C++ from C and can't afford a good book at the moment, [here's a link to Stroustrup's Tour of C++](https://isocpp.org/tour) My experiences with data structures courses in C++ is you're learning C. Understandable in this case because pretty much everything covered in a data structures course is already built in to C++, and "Just use the Standard Library" would make for a pretty dull course. – user4581301 Sep 07 '19 at 22:05

3 Answers3

5

std::vector has a constructor for this purpose:

std::vector<unsigned char> data(buffer, buffer + size);

newing a vector almost always should be avoided.

Live

Oblivion
  • 7,176
  • 2
  • 14
  • 33
3

The overload of the constructor of std::vector that you are using takes the number of elements to initialize the vector with and initializes these (to 0 in this case).

After the line

std::vector<unsigned char> *data = new std::vector<unsigned char>(size);

*data therefore contains already size elements set to zero.

Then with data->push_back(...) you are adding additional elements after these size elements. You are not overwriting the previous ones.

Either use

std::vector<unsigned char> *data = new std::vector<unsigned char>();

to default-initialize an empty vector, or use

(*data)[transfer] = ...

to set the elements at the given location.

Furthermore your program has undefined behavior if the fread reads less than size bytes into the array. You need to check the number of bytes read from its return value and you are not allowed to access any elements beyond that in data, because you never initialized it.

You can initialize it to zero with:

unsigned char* buffer = new unsigned char[size]{};

If you want to write C++, don't use C library functions like fread, use the facilities provided by <fstream>, i.e. std::ifstream and std::ofstream instead.

Similarly there is no need for dynamic memory allocation here. Declare variables with automatic storage:

unsigned char buffer[size]{};
std::vector<unsigned char> data(size);

and the rest of the syntax also simplifies:

data[transfer] = ...

Finally, as mentioned in the other answer, there is a constructor for std::vector that will perform the whole copy loop for you. Note however that my argument about undefined behavior still applies when using that.

Defining data as automatic array as in

unsigned char buffer[size]{};

works only if size is a compile-time constant. If it is not, then this part of my advice does not apply. However there is no real need to use arrays at all in any case. You can initialize a std::vector of proper size (compile-time constant or not) and provide that as buffer via its .data() method, which returns a pointer to the underlying (continuous) storage:

std::vector<unsigned char> buffer(size);
fread(buffer.data(), sizeof(unsigned char), buffer.size(), f);
walnut
  • 21,629
  • 4
  • 23
  • 59
  • 1
    `unsigned char buffer[size];` -whether this is legal C++ or not depends on what kind of thing `size` is. And it's completely unnecessary. –  Sep 07 '19 at 19:37
  • @NeilButterworth Added a paragraph. There is just too much that need to be mentioned here. – walnut Sep 07 '19 at 19:42
2

You don't need a separate buffer or any dynamic allocation in your code. You can create the std::vector with the desired size and then read from the file directly into the vector. The std::vector::data member function returns a pointer to the vector's underlying array that you can pass to the fread() function

std::vector<unsigned char> vec(size);
fread(vec.data(), sizeof(unsigned char), size, f);

Ideally you'll also check the return value from fread() to know how many elements were read.

Blastfurnace
  • 18,411
  • 56
  • 55
  • 70