Problems with copy constructor and dynamic memory

[JRevor]

I'm working with a dynamic vector class, called Vector. It has two attributes : longitud (which is the length) and p, which is a float pointer.

Here's the copy constructor :

Vector:: Vector(const Vector &v)
{
   longitud = v.longitud;
   p = new float[longitud];
   for (unsigned i = 0; i<longitud; i++)
   {
      at(i) = v.at2(i); 
   }
}

The copy constructor seems to work properly, everything is fine when tracing it from inside. However, after the copy is finished and returnet, the first component of the copied vector, p[0] stores a weird value for no apparent reason (3.99294e-34).

The problem seems to be solved if i change line 4 to this

p = new float[longitud+1];

What is going on? What am i doing wrong?
Thanks in advance

[hannes]

i don't think you can already use an instance of the class in the constructor in that class.

[helios]

i don't think you can already use an instance of the class in the constructor in that class.

Yes, you can.

The constructor looks good. Maybe there's something wrong with Vector::at()? And why do you have two at()s?

Holy crap. That was some déjà vu.

[JRevor]

Yeah, sorry about that helios. The copy constructor worked, but the whole thing isnt working properly. I'm a total newbie to C++,.. and consequently i'm terrible at things I used to know how to do in Free Pascal.

I really can't get where the error is. Im including the whole code (sorry for the lack of comments, they were in spanish, so i deleted them), just in case anything else is necessary. Go straight to the main() function. The problem is marked there.

#include  <iostream>
#include <new>
using namespace std;
/*-----------------------------------------------
Class Vector :
------------------------------------------------*/
class Vector
{
   private:
      float *p; 
      unsigned longitud; 
      float at2(unsigned i) const;  //value access. I need it for the copy constructor. If I use at(), i have a compilation error
   public:
      Vector (unsigned tam = 1); 
      ~Vector(); 
      unsigned length(void) const;  
      void print(void) const; 
      float& at(unsigned i); //Reference access
      Vector operator + ( Vector &a) ;
      Vector operator * ( float &a);
      Vector(const Vector &v); // copy constructor
};



float Vector::at2(unsigned i) const {return p[i];}  //returns the value

Vector:: Vector(const Vector &v)
{
   p = NULL;
   longitud = v.longitud;
   p = new float[longitud];
   for (unsigned i = 0; i<longitud; i++)
   {
      p[i] = v.at2(i); //Can't use at()  because vector &v is const
   }
  // return;
}

Vector Vector::operator * ( float &a)
{
   for (unsigned i = 0; i< longitud; i++)
   {
      p[i] = p[i]*a; 
   }
}

Vector Vector::operator + ( Vector &a)
{
   if (a.length() == length())
   {
      Vector temp(a.length());
      for (int i = 0; i < a.length(); i++)
      { 
         temp.at(i) = (a.at(i)+at(i));
         cout << a.at(i) << " + " <<at(i) <<" = "<< temp.at(i)<< endl;
      }
      return temp;       
   }
   else
   {
      cout << " +ERROR : Different components " ;
   }   
} 


/*-----------------------------------------------
Length : Returns the length
------------------------------------------------*/
unsigned Vector::length(void) const
{
   return longitud;
}
/*-----------------------------------------------
At : Returns a reference to a value
in the dynamic array
------------------------------------------------*/
float& Vector::at(unsigned i)
{
   return p[i];
}
/*-----------------------------------------------
Constructor 
------------------------------------------------*/
Vector::Vector(unsigned tam)
{
   p = NULL;	
   p = new float[tam];
   longitud = tam; 
   for (unsigned i = 0; i< longitud ; i++)
      p[i] = 1;
   return;
}

Vector::~Vector()
{
	delete [] p; 
	return;
}

void Vector::print(void) const
{
	for (unsigned i= 0; i<length(); i++) 
	{
		cout << "V[" << i<<"] = " << p[i] << endl ;	
	}
	cout << '\n';
	return;
}

int main (void) 
{
   //THIS WORKS
   Vector a(3); // It creates [1,1,1]
   Vector b(3);// idem
   Vector c(3); // idem
    
   //THIS DOESN'T REALLY WORK
   c = (a+b); //There's a problem here
   int d = 100;
   cout << "c" << endl; // C's first component goes nuts
   c.print();
   cout << "a" << endl; // however, a and b are OK
   a.print();
   cout << "b" << endl;
   b.print();
   return 0;
}

[helios]

Get rid of at2() and make at() const.
float& at(unsigned i) const;

There are a few problems with your code.
The first is that operator*() unintuitively modifies one of the operands and doesn't have a return statement.
Some control paths in operator+() don't return anything.
By default, a vector should be constructed empty. If it's not, then its elements should be zeroed.

Your problem is that you've defined the copy constructor, but that's a constructor, which means it's only called when the object is defined. In this case, on line 116. On line 119 you return a temporary copy of a Vector which is immediately destructed after being assigned to c. Since you didn't overload operator=(), which is what will be called here, the default one will be called. The default operator=() only performs a shallow copy; for example, pointers are copied without doing any allocation. Once the returned object is assigned to c, it is deleted, so c.p is now invalid. The call on line 122 dereferences an invalid pointer, and when c is destructed after main() returns c.p is deleted a second time, messing even more with the heap.

[JRevor]

Thank you so much helios. I think I'm in debt with you.

(by the way... i totally forgot about the operator*... i was gonna write down the operator overloading function, but went to the cafeteria and totally forgot about it. Also, thanks about mentioning about the paths in operator +)

Thanks a lot!!!