🔒 Locked

Tricky related class code!!

Started by adder_noir Nov 29, 2010 at 7:44 AM 10 replies 1.6k views

Original Post

Nov 29, 2010 at 7:44 AM

HI,!

Check out the following code I had a problem with a few days ago:

#include <iostream>#include <string>class Action{    public:    virtual ~Action(void){}    virtual void operator () (void) = 0; // both these must be implemented for    virtual Action* clone(void) const = 0; // this class to work};class Button{    public:    Button(const std::string& label)        : label_(label), action_(0) {}    void setAction(Action* newAction)    {        Action* temp = newAction->clone(); // this is the bit I'm puzzled by!        delete action_;        action_ = temp;    }    void onClick(void) const    {        if(action_){(*action_)();} // also this bit!    }    private:    std::string label_;    Action* action_;};class PlayMusic : public Action{    public:    PlayMusic(const std::string& songfile)        : song_(songfile) {}    ~PlayMusic(void)    {        return;    }    void operator()(void)    {        std::cout << "PlayMusic operator overload running" << std::endl;    }    Action* clone(void) const    {        std::cout << "PlayMusic clone function running" << std::endl;        return const_cast<PlayMusic*>(this); // I hacked this to get it to run!! ;o)    }    private:    std::string song_;};int main(){    Button myButton("MyButton");    PlayMusic myPlayMusic("MyPlayMusic");    myButton.setAction(&myPlayMusic);    return 1;}

I fixed the initial problem I had getting it to run which now allows me to turn my attention towards actually trying to work out what's going on. Basically the code is simulating what would happen if you had a button on a screen which played music when you clicked it. The author decided to use a class inheritance style function object. Quite crafty really, it means you can define one Button class and then define as many inherited varieties of the class Action as you like and Button will accept them all.

Great, but the code is not completed by the author and he's left some rather puzzling things in his wake. I'll start with the first things I'm puzzled by:

1)

void setAction(Action* newAction)    {        Action* temp = newAction->clone(); // this is the bit I'm puzzled by!        delete action_;        action_ = temp;    }

The setAction function in Button is odd. Why does it call the clone function in newAction to get a pointer to an Action instead of just using the pointer passed to it in the argument? What the hell does the clone function do, and why would its returned pointer take preference over the argument?

2)

void onClick(void) const    {        if(action_){(*action_)();} // also this bit!    }

A real puzzle. If I get this right, this function is checking to see if the action_ data member doesn't point to NULL and then dereferences the pointer presumably with a constructor call. Now that's ok if you're accessing the base class. But you can't access the base class. It has pure virtuals in it so is 'abstract' and is never meant to exist standalone. Given that all its inherited classes have a constructor call which take a string argument what exactly is going to happen when this function executes on the action_ data member when it's pointing to a derived class - in this example a class called PlayMusic (which takes a string as an argument)?

I do have more questions but that's enough for now! ANy insight anyone can offer would be most well received cheers! ;o)

Perost

Nov 29, 2010 at 8:05 AM

The answer to your first question is that Button uses a virtual constructor to make a copy of the action.

For you second question, it simply checks that action_ has been set to something with setAction. If it has, then it calls the operator() on the Action object (which has been overloaded by the class that inherits from Action, since operator() is declared pure virtual).

adder_noir

Nov 29, 2010 at 8:11 AM

That's a great reply. It will take me some time to mate what you've said to what's going on. For now I'd just like to say thankyou for the reply whilst I go away and crunch some concepts before returning later with a version of my own understanding to try and see if I've learned what you know properly. Thanks ;o)

Bregma

Nov 29, 2010 at 8:14 AM

Quote:
Original post by adder_noir
Why does it call the clone function in newAction to get a pointer to an Action instead of just using the pointer passed to it in the argument? What the hell does the clone function do, and why would its returned pointer take preference over the argument?

The clone() call clones the object pointed to. Your clone() will introduce undefined behaviour so you might want to change it.

A clone() is like a copy constructor only it's called something else, can be virtual, and has simpler syntax when invoked on a pointer-to-object. It is best implemented in terms of the copy constructor (ie. return new PlayMusic(*this);).

Quote:

void onClick(void) const    {        if(action_){(*action_)();} // also this bit!    }

That call simply invokes the operator()() member function of the object pointed to by _action. A straightforward member function call. Pretty standard stuff. Nothing fancy there. Not a constructor call at all.

Oh, and C++ does not use (void) to declare a function that takes no parameters. That's C, a different language.

SiCrane

Nov 29, 2010 at 8:38 AM

Quote:
Original post by Bregma
Oh, and C++ does not use (void) to declare a function that takes no parameters. That's C, a different language.

Hmmm? From ISO/IEC 14882:2003, the current version of the C++ standard, section 8.3.5 paragraph 2:

Quote:

The parameter list (void) is equivalent to the empty parameter list.

adder_noir

Nov 29, 2010 at 8:51 AM

Ok thanks I'm beginning to get somewhere with this thanks for the reply. I'm totally with part 2) now I've used this several times before, I can see it clearly now you've explained it cheers.

I understand what the clone thing does now thanks pal. What I don't get though is why it does it. Two things bother me:

1a)Why not just take the address of the argument passed in as is. Why the need to clone/copy construct it?

1b)If temp and thus action_ are assigned the value of the cloned class instance, won't they end up pointing to nothing when the function ends because they're taking the address of a local variable?

Man I have so far to go. I know you're right, just don't know why you are yet ;o)

**Edit** Bregma I rated you up but your rating didn't move. I rated you up recently too maybe there's a time delay thing?

rip-off

Nov 29, 2010 at 9:37 AM

Quote:

a)Why not just take the address of the argument passed in as is. Why the need to clone/copy construct it?

So the code will work even if the action passed is destroyed in the mean time:

void addAction(Button &button){   PlayMusic myPlayMusic("MyPlayMusic");   button.setAction(&myPlayMusic);} // original action is destroyed here...int main(){    Button myButton("MyButton");    addAction(myButton);    myButton.onClick(); // It is ok though, we've made a copy!}

Quote:

1b)If temp and thus action_ are assigned the value of the cloned class instance, won't they end up pointing to nothing when the function ends because they're taking the address of a local variable?

No, the return value of clone() should be a dynamically allocated value. Like Bregma said, it must be a new PlayMusic instance, you cannot just return the value of "this". So "temp" points at the new instance, the original action is deallocated and then action is pointed at the new instance.

This is exception safe code. You might think that it is a more complicated way of achieving the following:

delete action_;action_ = newAction->clone();

But what if clone() throws an exception? You now have a Button with a invalid action pointer, because it has been deleted but neither nulled out nor reassigned.

Quote:

Bregma I rated you up but your rating didn't move.

Once your rating gets high then it takes a higher rated member to move it. Ratings aren't time delayed, I believe.

adder_noir

Nov 29, 2010 at 11:18 AM

Quote:
Original post by rip-offNo, the return value of clone() should be a dynamically allocated value. Like Bregma said, it must be a new PlayMusic instance, you cannot just return the value of "this". So "temp" points at the new instance, the original action is deallocated and then action is pointed at the new instance.

This is exception safe code. You might think that it is a more complicated way of achieving the following:

*** Source Snippet Removed ***

But what if clone() throws an exception? You now have a Button with a invalid action pointer, because it has been deleted but neither nulled out nor reassigned.

Ok thanks again for a great post.

1)So if I've got this right, clone *must* return a pointer to a new dynamically allocated instance. So am I wrong to think that a dynamically allocated piece of memory - a new variable/object - is destroyed when a function ends even if it is created from within that same function? Surely it can't be destroyed like a locally declared variable or this wouldn't work.

2)What actually happens if clone() does throw an exception? I'd need to add something within the clone() function to return a NULL pointer right, or would I need to put that code somewhere else? Not too sure what to do about that to be honest. Am I correct to assume that any dynamic allocation needs an exception check and action plan to take if that happens?

I'm starting to understand this thanks! :o)

rip-off

Nov 29, 2010 at 3:23 PM

Quote:
Original post by adder_noir
1)So if I've got this right, clone *must* return a pointer to a new dynamically allocated instance. So am I wrong to think that a dynamically allocated piece of memory - a new variable/object - is destroyed when a function ends even if it is created from within that same function? Surely it can't be destroyed like a locally declared variable or this wouldn't work.

Dynamically allocated memory has a user controlled lifespan, rather than a scope controlled lifetime (unless one uses RAII techniques to manage the memory).

Dynamically allocating memory is the way to create an object that persists beyond its scope.

Quote:

2)What actually happens if clone() does throw an exception?

With the code as written, if clone() throws an exception, then the exception will be propagated to the caller, who can safely catch it and continue to use the Button object because it is exception safe. If the Button object is exception unsafe, then if you continued on you might find the Button invariants broken (the invariants in this case is that the "action_" pointer is either valid or NULL). Calling member functions in this state could cause undefined behaviour. Whether you catch the exception or not the Button is unsafe, even to destruct!

Quote:

I'd need to add something within the clone() function to return a NULL pointer right, or would I need to put that code somewhere else?

This is a common approach to error handling. There is some debate between the people who prefer exceptions and who return error return values, and those who lie somewhere in the middle. My thinking is that clone() typically only allocates memory, which has a small chance of failing. This is a pretty exceptional situation, and not one that can be handled locally often. I just leave the std::bad_alloc get thrown. You could mandate that clone can return NULL, but now all your code needs to take this into account to ensure that you have an if() check around every clone().

You will probably want to propagate the error to the caller too, because otherwise you'll end up with a bunch of buttons that won't work in the case that the memory limit has been reached. I'm sure the user would appreciate all that effort you went to and how you avoided crashing in the out of memory situation, only to be presented with a frozen GUI that they cannot do anything with!

Propagating the error requires yet more return codes. This isn't necessarily a bad thing, all I am saying is that if clone() can return NULL, then setAction should return an error status. If setAction returns an error status, then so must its callers, and so on.

adder_noir

Nov 29, 2010 at 3:51 PM

Quote:
Original post by rip-off
Propagating the error requires yet more return codes. This isn't necessarily a bad thing, all I am saying is that if clone() can return NULL, then setAction should return an error status. If setAction returns an error status, then so must its callers, and so on.

Thanks for a great and detailed reply.

1)On this particular point exactly where would this upward ripple effect ever stop? I'm worried that this means any part of code which has a dynamic allocation in it (especially in a big project) would need lots and lots of exception code written going way up the hierachy. In a game situation exactly where would this ever stop? The size of game engines is huge, I can only imagine how much writing bad allocation handling code would affect the writing of an engine and how would you deal with it anyway? Surely eventually this means at best a crash to desktop with an error message?

Scary!

2)I've been thinking about this and I'm come up with a horribly shoddy first effort at writing some exception code:

Action* clone(void) const{    std::cout << "PlayMusic clone function running" << std::endl;    return new PlayMusic("MyPlayMusic"); // for the time being I'm just loading in a string at this point}......void setAction(Action* newAction){    Action* temp = newAction->clone(); // this is the bit I'm puzzled by!    delete action_;    action_ = temp;    if(temp == NULL)    {        std::cout << "clone has returned NULL - memory allocation failure" << std::endl;    }}

I know it's seriously bad but it's a start. Here though as you say - if I've understood you - whatever calls setAction now also needs to know about this error or if in full screen mode say, the user will be in a locked up scenario and will be reaching for the task manager even though the bad allocation has been caught. I would guess this is the sort of 'upward cascade effect' you're talking about. Please do tell me anyone if I'm wrong!

I would imagine the only sensible thing to do here would be to access some sort of pre-defined critical error handling procedure/class which prints relevant details in a message box and returns the user to the desktop.

How am I doing so far? AM I making any sense? I feel a bit out on a barking limb here :oD

rip-off

Nov 29, 2010 at 5:46 PM

Quote:

On this particular point exactly where would this upward ripple effect ever stop? I'm worried that this means any part of code which has a dynamic allocation in it (especially in a big project) would need lots and lots of exception code written going way up the hierachy. In a game situation exactly where would this ever stop? The size of game engines is huge, I can only imagine how much writing bad allocation handling code would affect the writing of an engine and how would you deal with it anyway? Surely eventually this means at best a crash to desktop with an error message?

Well, if you use exceptions then the upward ripple is hidden until you choose to write a catch block. Whether this is a good thing depends on who you ask. Using new() or new[] in C++ cannot return null (unless you explicitly use std::nothrow), so you don't have to worry about it, the std::bad_alloc will be thrown and either caught somewhere you have chosen to handle it (handling bad_alloc correctly can be hard - remember you cannot rely on any direct or indirect allocation!) or will be passed on to the runtime (usually results in an ugly message box).

Quote:

Scary!

Indeed! Proper error handling is often scary.

Your #2 situation cannot occur because new will never return null. You'd have to do the following:

Action* clone(void) const{    return new(std::nothrow) PlayMusic("MyPlayMusic");}

You appear to be mixing up exception handling with general error handling. Exception handling is a compiler-supported method of handling errors. It is a little bit heavyweight, but can be very useful in certain cases. There is far from a consensus as to when it is best used.

adder_noir

Nov 29, 2010 at 5:53 PM

That's great thanks for your advice I can move on to the next chapter in my book now cheers ;o)

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.