In C++, is not permitted to pass an object to function directly?

Question

I know C, but I'm not good at C++.

The following code will crash (In getval(), using reference as a parameter is ok). And value of *p is changed after first cout statement. It looks there is some overwriting caused by out of bound of memory.

My question is why it crashed (or why its value is changed). It's 'call by value' of object, so should it work anyway?

class myclass { 
  int *p; 

  public: 
    myclass(int i); 
    ~myclass() { delete p; } 
    int getval(myclass o); 
}; 

myclass::myclass(int i) 
{ 
  p = new int; 

  if (!p) { 
    cout << "Allocation error\n"; 
    exit(1); 
  } 

  *p = i; 
}

int myclass::getval(myclass o) 
{ 
  return *o.p; 
} 

int main() 
{ 
  myclass a(1), b(2); 

  cout << a.getval(a) << " " << a.getval(b) << endl; 
  cout << b.getval(a) << " " << b.getval(b) << endl; 

  return 0; 
}

hint: what happens when your object gets copied and both copies get destryoed? — PlasmaHH, Oct 19 '11 at 08:04

score 6 · Answer 1 · edited May 23 '17 at 10:34

6

Read up on the rule of three.

Essentially, your code does not support copying by value.

So dynamically allocated memory is prematurely deallocated (by the destructor).

Cheers & hth.,

edited May 23 '17 at 10:34

Community

1
1

answered Oct 19 '11 at 08:09

Cheers and hth. - Alf

142,714
15
209
331

score 3 · Accepted Answer · answered Oct 19 '11 at 08:12

My question is why it crashed (or why its value is changed).

This is a very common problem of shallow copy and double delete. Compiler picks up the default copy constructor and both a.p and o.p are pointing to same memory location. When both objects invoke their destructor, the delete p; statement is executed twice. Freeing the same memory multiple times is an undefined behavior and it results in crash in your system.

It's 'call by value' of object, so should it work anyway?

If properly coded, then yes it would work. Make a deep copy of p's content and then it should work. However, it's better to pass by reference till it's possible.

score 2 · Answer 3 · answered Oct 19 '11 at 08:10

2

What you are doing is allowed as far as the language C++ is concerned. But that is really really bad programming. It dont want to comment on each line of your code. Instead, I re-write the whole code, and you compare this code with yours, and also see the comments embedded in the code:

class myclass { 
  int p;  //no pointer, as it is not needed

  public: 
    myclass(int i) : p(i) {} //use member initialization-list 
    int getval() const  //no parameter, and make the function const
    {
         return p;
    }
}; 

int main() 
{ 
  myclass a(1), b(2); 

  cout << a.getval() << endl;
  cout << b.getval() << endl; 
  return 0; 
}

answered Oct 19 '11 at 08:10

Nawaz

353,942
115
666
851

Personally, I'd assumed the example was actually more complicated, and this was a rare instance of competent question-asking and paring code down to a minimal repro case...! – HostileFork says dont trust SE Oct 19 '11 at 13:09
This code really looks cleaner. I din't notice that while answering. Nice one. – iammilind Oct 19 '11 at 13:59

score 1 · Answer 4 · answered Oct 19 '11 at 08:12

1

You are passing myclass as value, so copy is created. When myclass::getval returns the stack is unrolled and myclass destructor is called, releasing the memory that pointer points to in the other object.

answered Oct 19 '11 at 08:12

Milan Babuškov

59,775
49
126
179

score 0 · Answer 5 · answered Oct 19 '11 at 08:09

0

You are double deleting p here, a shared pointer may be useful if you dont want to do a deep copy. As long as its possible and makes sense try to use automatic variables instead (int instead of int*)

answered Oct 19 '11 at 08:09

atamanroman

11,607
7
57
81

score 0 · Answer 6 · answered Oct 19 '11 at 08:10

You haven't implemented the copy constructor (if you do, keep in mind the rule of three), so when copies are destroyed, you free the same memory multiple times.

Hints:

Implement the copy constructor:

myclass(const myclass& other)
{
   p = new int;
   *p = *other.p;
}

Pass by reference:

int getval(const myclass& o);

A good rule would also be to make functions that don't depend on the object itself static (this won't help you in this particular case, but useful nonetheless). Since getval doesn't access any object members, you can make it static.

Reasons for the crash:

When you call getval(o), a copy of o is created and its member points to the same location as the original object, since you didn't declare a copy constructor. The copy is destroyed and the memory is deleted. The original object is left corrupted.

In C++, is not permitted to pass an object to function directly?

6 Answers6