6

As part of an assignment we have been asked to create a Vector3D class which uses memory allocated on the Heap. I have a Vector3DHeap class with the following constructor.

Vector3DHeap::Vector3DHeap(float& x, float& y, float& z)
{
    this->x = &x;
    this->y = &y;
    this->z = &z;
}

If I want to get a unit vector, I was expecting the be able to do the following. This gives the error message "No instance of constructor matches the argument list, argument types are (float, float, float).

Vector3DHeap* Vector3DHeap::getUnitVector()
{
    float m = *getMagnitude();

    return new Vector3DHeap((*x / m), (*y / m), (*z / m));
}

The compiler is happy if I define three float variables, a, b and c and pass these to the constructor. What is wrong with the code above?

Vector3DHeap* Vector3DHeap::getUnitVector()
{
    float m = *getMagnitude();

    float a, b, c;

    a = *x / m;
    b = *y / m;
    c = *z / m;

    return new Vector3DHeap(a, b, c);
}

Many thanks, George

  • 1
    You can't bind a temporary to a non-const reference. – chris Oct 20 '13 at 15:29
  • Your constructor should have `const float&` arguments. – n. m. could be an AI Oct 20 '13 at 15:33
  • @n.m. He says he is storing data on the heap, so x, y, z are in fact pointers. I don't know why you would want to do that but there are certainly some implementations where it can be useful. He cannot use constant reference because then he would not be able to assign them to the (non-constant) pointers. – N A Oct 20 '13 at 15:38
  • @DanielSaska `Vector` using memory allocated on the heap is different from vector holding pointers to memory allocated on the heap by someone else. What OP needs to do is have the vector allocate some memory, then use that memory to store some values. – juanchopanza Oct 20 '13 at 15:44

3 Answers3

13

Your problem with the first version is that your compiler is trying to prevent a bug.

Your problem with the second version is that you outsmarted your compiler and successfully managed to create a bug.

Given your constructor, you want to store pointers to the float values that are passed by reference. Since your second version now calls the constructor with references to the local variables float a, b, c;, you created an instance of Vector3DHeap which references them. But as soon as getUnitVector returns, those variables no longer exist and the references stored in Vector3DHeap became dangling references.

The solution is not to store pointers inside Vector3DHeap or to create copies of the parameters:

Vector3DHeap::Vector3DHeap(float x, float y, float z)
{
    this->x = new float(x);
    this->y = new float(y);
    this->z = new float(z);
}

Make sure that you properly delete the stored floats, though.

Daniel Frey
  • 55,810
  • 13
  • 122
  • 180
1

It is good that the compiler stopped you from binding a reference to a temporary because otherwise you would have ended up with an object pointing to already destroyed objects: The expressions *x / m and similar each yield a temporary float object which will disappear at the end of the expression. Trying to bind a temporary to a non-const reference will fail.

However, I doubt that you really want to do any of that: you shouldn't use pointers unless you really know that you need to use pointers! Your constructor should probably rather look like this:

Vector3DHeap::Vector3DHeap(float x, float y, float z)
    : x(x), y(y), z(z) {
}

where the members are, of course, also of type float. getMagnitude() should return a float, too. ... as should getUnitVector() return a Vector3DHeap rather than a pointer to it!

Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
  • Thanks for your reply. We have been asked to "rewrite the class (call it Vector3DHeap) so that variables are stored on the heap rather than the stack." If return a float and a Vector3DHeap rather than a pointer, will those not be on the Stack instead of the Heap? –  Oct 20 '13 at 15:44
  • @GeorgeRobinson Objects are stored on the heap whenever you use `new` or `new[]` to initialize them. – David G Oct 20 '13 at 15:54
  • I must return a pointer type if I have return new Vector3DHeap(x, y, z) since new returns a pointer? I am confused since you said "getUnitVector() return a Vector3DHeap rather than a pointer to it!" –  Oct 20 '13 at 15:57
  • @GeorgeRobinson: Mandating to store individual `float`s on the heap is _extremely_ stupid and I certainly hope your teacher did **not** mean that! Even if the objects need to be on the heap you'd allocate (and realese) them by a suitable class: returning pointer for objects not maintained otherwise generally dies not work. – Dietmar Kühl Oct 20 '13 at 17:01
1
  • (*x / m) is a temporary object.
  • Vector3DHeap(float& x, float& y, float& z) requires a non-const reference as first parameter.

You can't pass a temporary object to a function that expects a non-const reference. See https://stackoverflow.com/questions/13826897#13827042 for details why C++ does not want to allow this.

Community
  • 1
  • 1
Oswald
  • 31,254
  • 3
  • 43
  • 68