C++ Overloading << Operator problem

Question

I'm fairly noobish at C++, but very comfortable with pointers, dereferencing, etc. I'm having a problem with my overload of the << operator for a class, in that it compiles fine but crashes when run. It feels like an infinite loop, but I'm not certain. Here's the code, and any help is appreciated.

#include <string>
#include <iostream>

using namespace std;

class Person
{

private:

 string _name;
 Person* _manager;

public:

 Person(string name, Person *manager);
 Person(string name);

 friend ostream &operator<<(ostream &stream, Person &p);

};


Person::Person(string name, Person *manager)
{
 _name = name;
 _manager = manager;
}

Person::Person(string name)
{
 _name = name;
}

ostream &operator<<(ostream &stream, Person &p)
{
 Person* mgr = p._manager;

 stream << p._name << std::endl;
 stream << mgr->_name << std::endl;
 return stream;
}


int main()
{
 Person *pEmployee = new Person("John Doe Employee");
 Person *pManager = new Person("John Doe Manager", pEmployee);

 cout << *pEmployee;
 cout << *pManager;

 return 0;
}

camh · Accepted Answer · 2009-10-14 07:48:55Z

4

In your constructor with just a single argument, you need to set _manager to NULL/0. Test for this in your operator<< and don't output mgr->name if it is NULL/0. As it stands, you are dereferencing an uninitialised pointer.

Person::Person(string name)
{
    _name = name;
    _manager = 0;
}

ostream &operator<<(ostream &stream, Person &p)
{
    Person* mgr = p._manager;

    stream << p._name << std::endl;
    if (mgr)
        stream << mgr->_name << std::endl;
    return stream;
}

There are a number of other things you could do better, such as using const references on arguments and using the constructor initialiser list, but they wont be the cause of your problem. You should also address ownership issues with the manager object you pass in to the constructor to ensure it does not get double-deleted.

answered Oct 14, 2009 at 7:48

camh

42.9k13 gold badges66 silver badges73 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

dreadwail Over a year ago

D'oh! Thanks a ton. Seems when its late I forget the simplest of things.

David Rodríguez - dribeas Over a year ago

Since you are at it: use initialization lists and default parameters: explicit Person( std::string const & name, Person * manager = 0 ) : _name( name ), _manager(manager) {}. The default parameter allows you to call with a single argument, the explicit keyword disables implicit conversions from std::string to Person by means of constructing a Person with a single std::string

stijn · Accepted Answer · 2009-10-14 07:49:23Z

2

your _manager pointer is not initialized upon constructing the first Person instance, hence referencing p._manager in your operator << crashes. Besides that, you have a memory leak since you call new but not delete.

answered Oct 14, 2009 at 7:49

stijn

36.1k13 gold badges119 silver badges167 bronze badges

2 Comments

Ferdinand Beyer Over a year ago

It's fine to use new without delete in the main method. One main returns, the memory is reclaimed by the operating system. There is no memory leak here.

stijn Over a year ago

sure, but it's good practice to match each new with a corresponding delete ;P but in this case I'd rather put it all on the stack anyway.

Ferdinand Beyer · Accepted Answer · 2009-10-14 07:56:36Z

Your implementation of operator<< does not check if the _manager member is valid. If there is no manager, you should make this explicit by setting the pointer to 0. Otherwise the pointer value is undefined and accessing it will crash your program.

(1) Your Person class should set _manager to 0 (null pointer) if none is supplied in the constructor:

Person::Person(string name) : _name(name), _manager(0)
{
}

(2) In your operator<<, check the pointer before dereferencing it:

if (mgr) {
    stream << mgr->_name << std::endl;
}

Some hints for better code:

(1) Change your function arguments to accept const string& instead of string. This way, the string is not copied when calling the function/constructor, but passed as a constant reference.

(2) It is cleaner to let your operator<< accept a const reference for the Person as well, since it does not/should not modify the Person. This way, you can use the operator also in places where you have a constant Person.

Daniel Daranas · Accepted Answer · 2009-10-14 07:51:21Z

0

Person::Person(string name)
{
    _name = name;
}

Who is your manager?

Person::Person(string name) : _manager(NULL)
{
    _name = name;
}

ostream &operator<<(ostream &stream, Person &p)
{
 Person* mgr = p._manager;

 stream << p._name << std::endl;
 if (mgr != NULL) { stream << mgr->_name << std::endl; }
 return stream;
}

answered Oct 14, 2009 at 7:51

Daniel Daranas

22.7k10 gold badges66 silver badges122 bronze badges

Comments

swegi · Accepted Answer · 2009-10-14 07:50:19Z

-1

Your Person instance *pEmployee does not have the _manager set. This may be a NULL pointer.

answered Oct 14, 2009 at 7:50

swegi

4,1221 gold badge28 silver badges45 bronze badges

2 Comments

Ferdinand Beyer Over a year ago

When he is not explicitely setting it to NULL, it is very unlikely that it is a null pointer. (Note that I did not vote you down for this :-))

swegi Over a year ago

That's why I wrote 'may' ;-) In fact, dangling pointers are much worse IMO.

Collectives™ on Stack Overflow

C++ Overloading << Operator problem

5 Answers 5

2 Comments

2 Comments

Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

2 Comments

2 Comments

Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related