0

It's likely that what I'm trying to do is terrible practice. If so please tell me so I can stop. Anyway, I'm working on some basic matrix math functions using dynamic memory to store the matrix data array. I'm trying to figure out how to convert a vector passed into a constructor to this array, but I get some odd results. Here's the simple version.

Header:

class Matrix {
public:
    unsigned int size = 4;
    float *M;

    Matrix(std::vector<float> values) : M(&values[0]){}

    ~Matrix() = default;
};

And the test file:

int  main(){
  Matrix m(std::vector<float>{ 0.0f, 0.0f, 0.0f, 1.0f,
                               1.0f, 2.0f, 3.0f, 4.0f,
                               5.0f, 6.0f, 7.0f, 8.0f,
                               9.0f, 10.0f, 11.0f, 12.0f });
  return 0;
}

The result is...wrong. Everything seemingly works (from watching the values while debugging) until the return from the constructor. My guess is Visual Studio's debugger is lying to me though and it happens earlier than that.

So aside from the fact that the size isn't being changed, what's going on? Is there a better way to do this? Also, is it possible to accept a std::initializer_list and give that data to the dynamic pointer?

Alex Coats
  • 115
  • 2
  • 16

2 Answers2

2

The vector you're passing in is a temporary object. Once the constructor finishes the temporary vector will cease to exist, and thus the address you stored (&values[0]) will thus point to garbage. Your code may in fact work, but it also may in fact crash your computer, or make you coffee, or do any other undefined thing.

If you want to keep the same interface (i.e. passing in a vector) you need to copy the data out. Something like this will get you started

Matrix(std::vector<float> values) : M(new float[values.size()])
{
    std::copy(values.begin(), values.end(), M);
}

but now you have to free M in your destructor, and implement proper copy constructors and assignment operators. Also don't forget your move constructors and assignment operators too ;) Look into std::unique_ptr and std::shared_ptr (and of course std::vector) for better ways of managing memory.

bstamour
  • 7,746
  • 1
  • 26
  • 39
  • I will add that once you fix the argument to take a reference, you will be pointing your pointer var M to the internal memory of the vector. Any change in there is going to screw up the vector. Also, note that the vector might be carrying extra memory (it allocates more memory than required to grow dynamically). – Shyamal Desai Nov 18 '15 at 21:37
  • All true. I don't want to overload the OP with too many details though. The essence of the issue is he/she is holding a pointer to something temporary. – bstamour Nov 18 '15 at 21:38
  • Thanks man, that was super quick! Unfortunately I have tried to do what you suggested already and std:copy has some safety issues. Not really sure enough about it's usage to determine what's wrong. However, I will definitely look into these pointers (heh). Would you say using vector over arrays would represent a significant performance drop with a lot of matrices? – Alex Coats Nov 18 '15 at 21:50
  • There should be no performance drops with moving to vectors. Under the hood, vector is just a pointer and a size. It's only when you reallocate it (i.e. add new elements) that you might experience some slowdown, but once it's filled to the right size then it should be just as fast as a raw array. – bstamour Nov 19 '15 at 14:03
0

With your current code, you are storing a dangling pointer. The vector and its data will be destructed after the call to the constructor of Matrix.

You can solve the problem with one of the following options.

Option 1

Don't use raw pointers. Use std::vector<float> instead. Replace

float *M;

by

std::vector<float> M;

Then, you can update the constructor to be:

Matrix(std::vector<float> values) : M(values){}

Option 2

Allocate memory for M and then copy the data from the input argument in the constructor.

Matrix(std::vector<float> values) : M(new float[values.size()])
{
   std::copy(values.begin(), values.end(), M);
}

If you use this option, you'll need to implement a proper copy constructor, a copy assignment operator, and a destructor. Remember to read up on The Rule of Three.

Community
  • 1
  • 1
R Sahu
  • 204,454
  • 14
  • 159
  • 270
  • Option 1 is certainly something I've thought about. I was hoping to use the lightest weight data structure though, and I'm not sure whether the performance differences would be significant. – Alex Coats Nov 18 '15 at 21:53
  • @AlexCoats, I'll be very surprised if use of `std::vector` has any noticeable adverse impact on performance. – R Sahu Nov 18 '15 at 21:54