surprising constructors for a class!

Question

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

I had also given upvotes for answers. I always reward people who answers. — , Aug 18 '10 at 12:38
`std::vector` works, why using anything else for the same job ? — Matthieu M., Aug 18 '10 at 13:58

score 4 · Answer 1 · answered Aug 18 '10 at 12:06

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;

score 2 · Answer 2 · edited Dec 22 '11 at 03:30

2

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=.

edited Dec 22 '11 at 03:30

Anthony Pegram

123,721
27
225
246

answered Aug 18 '10 at 11:57

Eiko

25,601
15
56
71

Imho, upvoting a accepted answer is not **required**. Sometimes the only answer that gives the correct solution but is poorly written or written with a disrespectful tone and it doesn't deserve an upvote. However I totally agree with the fact that at least accepting good answers is mandatory. – ereOn Aug 18 '10 at 12:07
but for v1 = x, second constructor is being called; checked with gdb. I was also thinking about operator=, but now I am not getting what is happening here and why so! – Aug 18 '10 at 12:10
@Srin operator= seems the thing you want as in my answer below. But in general perhaps you should rethink the whole strategy. – Elemental Aug 18 '10 at 12:14

score 1 · Accepted Answer · answered Aug 18 '10 at 12:10

1

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];

answered Aug 18 '10 at 12:10

Elemental

7,365
2
28
33

You seems to be correct. But how v1=x; becomes v1=vector(x);? Is there any rule in standard? If such a serous mistake is there in code, then compiler must catch it...am I wrong? – Aug 18 '10 at 12:31
@svin The compiler needs to match the operator =; the first thing it looks for is: Vector::operator=(int *) [which you don't have] So it looks at the assignments available and finds vector::operator=(vector&) [which is generated by the compile] In order to make this operator work it needs a vector on the RHS and PRESTO it notices that you have defined vector(int*) so it can make vector from a int array - so it does. This stuff is confusing until you really get it - looking up the rule of three (and especially the reasons why such guidelines exist) might help you some. – Elemental Aug 18 '10 at 12:52

score 0 · Answer 4 · answered Aug 18 '10 at 12:09

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

score 0 · Answer 5 · edited May 23 '17 at 12:09

0

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.

edited May 23 '17 at 12:09

Community

1
1

answered Aug 18 '10 at 12:57

FireAphis

6,650
8
42
63

score 0 · Answer 6 · answered Aug 18 '10 at 15:40

  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).

surprising constructors for a class!

6 Answers6