CODEWITHSUNDEEP
c++
lisa
lisa

Reputation:

auto_ptr question in c++

I am new here. I am also new on C++ So here is the class and function i wrote.But i got the compiler error My class:

class fooPlayer
{
public:
       void fooPlayerfunc(){}//doing something here
       char askYesNo(std::string question);
};

class fooPlayerFactory
{
public:
   virtual std::auto_ptr<fooPlayer> MakePlayerX() const;
   virtual std::auto_ptr<fooPlayer> MakePlayerO() const;
private:
   std::auto_ptr<fooPlayer> MakePlayer(char letter) const;
   std::auto_ptr<fooPlayer> my_player;

};

Implement my class:

auto_ptr<fooPlayer> fooPlayerFactory:: MakePlayer(char letter) const
{
       my_player->fooPlayerfunc();
       return my_player;
}

auto_ptr<fooPlayer> fooPlayerFactory::MakePlayerX() const
{
      char go_first = my_player->askYesNo("Do you require the first move?");
      MakePlayer(go_first);
      return my_player;
}

auto_ptr<fooPlayer> fooPlayerFactory::MakePlayerO() const
{
    return my_player;
}

My main() function here:

int main()
{
          fooPlayerFactory factory;
          factory.MakePlayerX();
          factory.MakePlayerO();
}

I got the error: error C2558: class 'std::auto_ptr<_Ty>' : no copy constructor available or copy constructor is declared 'explicit'

I do not know how to change it even after reading the document on this link:

Upvotes: 2

Views: 2005

Answers (6)

Max Lybbert
Max Lybbert

Reputation: 20047

I'm not seeing anywhere you construct my_player, so I have a feeling that some of the code is missing. Specifically, I think your constructor has this line:

my_player = new fooPlayer()

A fooPlayer object is not quite the same thing as an auto_ptr<fooPlayer> object, and auto_ptr is intentionally designed to prevent assigning from one to the other because, frankly, the alternative is worse. For the details, look up (1) conversion constructors, (2) the explicit keyword, and (3) copy constructors and destructive copy semantics.

You should change the constructor to either:

class fooPlayerFactory {
    public:
    fooPlayerFactory()
    {
        my_player = std::auto_ptr<fooPlayer>(new fooPlayer());
    }

Or (using a member initializer list):

class fooPlayerFactory {
    public:
    fooPlayerFactory() : my_player(std::auto_ptr<fooPlayer>(new fooPlayer()) { }

The solution isn't pretty but, like I said, the alternative is worse due to some really arcane details.

As a bit of advice, though, you're making life harder than it needs to be; and may in fact be causing strange bugs. auto_ptr exists to manage the lifetime of an object, but the only reason you need to worry about the lifetime of my_player is that you've allocated it with new. But there's no need to call new, and in fact there's no need to keep my_player. And unless fooPlayerFactory is meant to be the base class for some other factory, there's no need to mark functions virtual.

Originally I thought you could get away with simply returning copies of the my_player object, but there's a problem: before returning my_player from MakePlayer() you call a method on it, and I assume that method changes the internal state of my_player. Further calls to MakePlayer() will change the state again, and I think you're going to eventually have my_player in the wrong state. Instead, return a different fooPlayer object with each request. Don't do memory management, just promise to construct the object. That way the user can decide on memory allocation:

fooPlayerFaclotry factory;
fooPlayer on_stack = factory.MakePlayerX();
fooPlayer* on_heap_raw_pointer = new fooPlayer(factory.MakePlayerO());
std::auto_ptr<fooPlayer> on_heap_managed_scope
                         = std::auto_ptr<fooPlayer>(factory.MakePlayerX());

I would change fooPlayerFactory to look like this:

class fooPlayerFactory
{
private:
   fooPlayer MakePlayer(const char letter) const
   {
       fooPlayer result;
       result.fooPlayerfunc();
       return result;
   }

public:
   fooPlayer* MakePlayerX() const
   {
       char go_first = askYesNo("Do you require the first move?");
       return MakePlayer(go_first);
   }

   fooPlayer MakePlayerO() const
   {
       return fooPlayer();
   }
};

Upvotes: 0

piotr
piotr

Reputation: 5745

Also your code is buggy, any class that implements virtual methods should have a virtual destructor.

Upvotes: 0

iain
iain

Reputation: 10928

The reason for the error is that you are calling the copy constructor of auto_ptr my_player in fooPlayerFactory::MakePlayerO() which is a const method. That means that is cannot modify its members.

However the copy constructor of auto_ptr DOES modify the right hand side so returning my_player trys to change its pointer to 0 (NULL), while assigning the original pointer to the auto_ptr in the return value.

The signature of the copy constuctor is

auto_ptr<T>::auto_ptr<T>(auto_ptr<T> & rhs)

not 

auto_ptr<T>::auto_ptr<T>(const auto_ptr<T> & rhs)

The copy constructor of auto_ptr assigns ownership of the pointer to the left hand side, the right hand side then holds nothing.

I don't think you want to use auto_ptr here, you probably want boost::smart_ptr

It looks like you have mixed up two uses for auto_ptr

The first is as poor man's boost::scoped_ptr. This is to manage a single instance of a pointer in a class, the class manages the life time of the pointer. In this case you don't normally return this pointer outside your class (you can it is legal, but boost::smart_ptr / boost::weak_ptr would be better so clients can participate the life time of the pointer)

The second is its main purpose which is to return a newly created pointer to the caller of a function in an exception safe way.

eg

auto_ptr<T> foo() {
    return new T;
}

void bar() {
    auto_ptr<T> t = foo();
}

As I said I think you have mixed these two uses auto_ptr is a subtle beast you should read the auto_ptr docs carefully. It is also covered very well in Effective STL by Scott Meyers.

Upvotes: 5

Loki Astari
Loki Astari

Reputation: 264739

I don't think you actually want to constructing dynamic objects here.
A factory object creates and returns an object it normally does not keep a reference to it after creation (unless you are sharing it), and I don't actually see anywhere that you are creating the player.

If you only ever create one player internally in your (fooPlayerFactory). Then create an object and return references to it.

Upvotes: 2

anon
anon

Reputation:

In your code:

auto_ptr<fooPlayer> fooPlayerFactory:: MakePlayer(char letter) const
{
       my_player->fooPlayerfunc();
       return my_player;
}

This is a const function, but fooPlayerfunc is not const - my compiler reports this error rather than the one you say you are getting. Are you posting the real code?

Upvotes: 2

thesamet
thesamet

Reputation: 6582

Edit: in response to the comment (which is correct, my bad), I left only the advice part.

Best practice is to have the factory methods just return a plain old pointer to the underlying object, and let the caller decide how to manage ownership (auto_ptr, scoped_ptr, or whatever).

Upvotes: 1

Related Questions