surprising constructors for a class!

Question

Hi,

What is the problem here in this code? It gives segmentation fault. I found value of size in vector (int *a) is no more 3. How is this?

 #include <iostream> 

 using namespace std; 

 class vector 
 { 
  int *v; 
  int size; 
     public: 
  vector(int m) 
  { 
   v = new int[size = m]; 
   for(int i=0; i<size; i++) 
    v[i] = 0; 
  } 
  vector (int *a) 
  { 
   for(int i=0; i<size; i++) 
    v[i] = a[i]; 
  } 
  int operator*(vector &y) 
  { 
   int sum = 0; 
   for(int i=0; i<size; i++) 
    sum += this -> v[i] * y . v[i]; 
   return sum; 
  }    
 }; 


 int main() 
 { 
  int x[3] = {1,2,3}; 
  int y[3] = {4,5,6}; 
  vector v1(3); 
  vector v2(3); 
  v1 = x; 
  v2 = y; 
  int R = v1 * v2; 
  cout << "R = " << R; 
  return 0; 
 } 

Sincerely,
Srinivas Nayak

Answer 1

This is not surprising at all.

Your first constructor seems ok, the second one does miss the allocation for v.

Edit: v1 = x and v2 = y doesn't make sense without overloading operator=.

PS: Please up-vote and accept good answers, your accept rate is currently 0% making it unattractive to answer. Please also review your old questions. Thanks.

Answer 2

Apart for the problem with the allocation in your constructor vector (int *a), you also need an overloaded assignment operator:

    int operator=(int *a) {
            for(int i=0; i<size; i++)
                    v[i] = a[i];
    }

Since the following two make use of it:

v1 = x; 
v2 = y;

Answer 3

I hope this is homework!
Otherwise you should be using std::Vector

Couple of problems:

• A constructor should initialize all members.
  • Neither constructor does this.
• The copy constructor (or what seems to be) is not defined incorrectly.
• Because your class manages a RAW pointer you need to see "The rule of three"
  • ie you are missing the assignment operator
• Prefer to use initializer lists when you can

Answer 4

In essence the reason it generates a fault is in the line v1=x;

As you have no assignment operator this in effect becomes: v1=vector(x) Which called your int * constructor. This constructor runs with size initialised to garbage which causes the seg fault as the loop progresses towards invalid memory.

Strategically the problem is you want create a new object for a int * but you don't know how big the array is that you are pointing at.

Your code looks like you want to assume that the array is the correct size for the currently defined vector in which case the operator you want to define this function in preference to the constructor: operator=(int *)

You generally seem a bit confused about which object is which for example sum += this -> v[i] * y . v[i]; would normally jusy be written as in this context sum += v[i] * y . v[i];

Answer 5

In addition to all the correct answers here, consider adding keyword explicit before all your constructors that accept only one argument. That way it will never be confused with assignment operations. Here's another question that explains it.

Answer 6

  vector (int *a) 
  { 
   for(int i=0; i<size; i++) 
    v[i] = a[i]; 
  } 

This constructor couldn't possibly work.

• The v member is not initialized. You have not allocated storage for the values.

• The size member is uninitialized. The loop will try to read an undeterminate amount of values from the passed pointer.

• There is no way to initialize size. If you are given just an int*, there is no way to determine how large the pointed array is (if the pointer even points into an array in the first place). This is the reason, why the number of elements in the array has to be passed separately (and why plain arrays are a PITA).