Help with redesign, please.

[prestokeys]

Using the proxy pattern was JLBorges' recommendation for a specific need in my program, and it has been working beautifully until the following scenario is reached:

#include <iostream>
#include <string>
#include <memory>

class Person;  class PersonProxy;

class PersonInterface {
	public:
		virtual ~PersonInterface() = default;
		virtual PersonProxy* getProxy() const = 0;
		virtual void createProxy (Person*) = 0;
};

class Person : public PersonInterface {
	private:
		std::string name;
		std::shared_ptr<PersonProxy> proxy;
	public:
		Person() = default;
		explicit Person (const std::string& name) : name (name) {}
	public:
		virtual PersonProxy* getProxy() const override {return proxy.get();}
		inline void createProxy (Person* p);
};

class PersonProxy : public PersonInterface {
    private:
		std::shared_ptr<Person> actual;
	public:
	    explicit PersonProxy (Person* p) : actual (std::shared_ptr<Person>(p)) {}
		explicit PersonProxy (std::shared_ptr<Person> p) : actual (p) {}
		void rebind (std::shared_ptr<Person> p) {actual = p;}
		virtual PersonProxy* getProxy() const override {return actual->getProxy();}
		virtual void createProxy (Person* p) override {actual->createProxy(p);}
};

class Girl : public Person, public std::enable_shared_from_this<Girl> {
	public:
		Girl (const std::string& name) : Person (name) {createProxy (this);}
};

inline void Person::createProxy (Person* p) {
	proxy = std::shared_ptr<PersonProxy>(new PersonProxy(p));
}

int main() {
	{
		Girl* a = new Girl("a");
		std::shared_ptr<Girl> b = std::make_shared<Girl>("b");
		a->getProxy()->rebind(b);
		std::cout << "rebind succeeded." << std::endl;
	}
	std::cout << "Exited scope." << std::endl;
        // Crash (with VS2013) because b is deleted twice due to two separate ownership groups.
}

I tried using std::enable_shared_from_this<Girl>, but that only made it worse. Should I use weak_ptr? So far the only problem is the double deletion in scenarios like the above, but it must be removed. I still want to keep using proxies though, because it takes care of all the program's needs and to keep using smart pointers for the proxy and actual.

PersonProxy is needed to allow for address changes when one person transforms into another type through fantasy-related stuff, and then keeping the same relations among the Person objects after such transformations, and the proxy pattern here carries out the function as intended. For example, Mark loves Cindy, and then Cindy turns into a dog--a totally different class. Guess what? Mark now loves the dog (and will try to change the dog back to class Girl).

[cire]

// Crash (with VS2013) because b is deleted twice due to two separate ownership groups.

You sure about that? I suspect nothing is getting deleted at all here, what with the circular references and shared ownership. Why does a person own a proxy? Why is a Person's constructor responsible for creating a proxy? If you rebind the proxy which was created by *a, *a still shares ownership of that proxy. Any calls to getProxy on *a will result in *a returning a proxy which references b.

Why does Person even need to know about PersonProxy?

[prestokeys]

PersonProxy is needed to allow for address changes when one person transforms into another person through fantasy-related stuff, and then keeping the same relations among the Person objects after such transformations (long story kept short), and the proxy pattern here carries out the function as intended.

Different compilers are saying different things. GCC seems to think that the references are circular and so nothing is deleted, while Visual Studio 2013 thinks that each reference count has dropped to 0. Anyways, something smells afoul here so I've temporarily made Person::proxy and PersonProxy::actual into regular pointers, and everything is crash-free now, but the inevitable leaking going on leaves the design in a bad state still.

[JLBorges]

> ou sure about that? I suspect nothing is getting deleted at all here,

The problem here is that the only Girl object gets destroyed too early; two, unrelated, independent shared pointers try to manage the same object. When the first one is reset, it destroys the object; the destructor of the second shared pointer tries to destry an object that has already been destroyed engendering undefined behaviour.
http://rextester.com/IMN7906



std::enable_shared_from_this<> is used when

a. we want to generate additional correct shared pointers to objects from within member functions.
(because this is never a shared pointer)

b. std::enable_shared_from_this<>::shared_from_this() is used for this purpose; it can be used on an object if and only if the object already has at least one shared pointer that manages it.

See the notes and the Good/Bad example: http://en.cppreference.com/w/cpp/memory/enable_shared_from_this

I've no recollection whatsoever of the earlier program which used proxies; but one way to manage shared ownership of objects correctly would be along these lines:

#include <iostream>
#include <string>
#include <memory>

class Person;  class PersonProxy;

class PersonInterface
{
    public:
        virtual ~PersonInterface() = default;
        virtual std::shared_ptr<PersonProxy> getProxy() = 0;
};

class Person : public PersonInterface, public std::enable_shared_from_this<Person>
{
        std::string name;

    protected: // sub-objects are constructible by derived classes
        Person() = default;
        explicit Person( const std::string& name ) : name( name ) {}

    public:
        virtual std::shared_ptr<PersonProxy> getProxy() override ;
};

class PersonProxy : public PersonInterface, public std::enable_shared_from_this<PersonProxy>
{
        std::shared_ptr<Person> actual;
        explicit PersonProxy( std::shared_ptr<Person> p ) : actual( p ) {}

    public:
        // enforce dynamic storage duration
        static std::shared_ptr<PersonProxy> make(std::shared_ptr<Person> p) {
            return std::shared_ptr<PersonProxy>( new PersonProxy(p) ); }

        void rebind( std::shared_ptr<Person> p ) { actual = p; }

        virtual std::shared_ptr<PersonProxy> getProxy() override 
        { return actual->getProxy() ? actual->getProxy() : shared_from_this() ; }
};

std::shared_ptr<PersonProxy> Person::getProxy() {
    return PersonProxy::make( shared_from_this() ) ;
}
class Girl : public Person
{
    Girl( const std::string& name ) : Person( name ) { 
        std::cout << "Girl::constructor: " << name << ' ' << this << '\n' ;
    }

    public:
        // enforce dynamic storage duration
        static std::shared_ptr<Girl> make(const std::string& name) {
            return std::shared_ptr<Girl>( new Girl( name ) ) ;
        }
        
        ~Girl() { 
            std::cout << "Girl::destructor: " << this << '\n' ; 
        }
};

int main()
{
    {
        auto a = Girl::make( "a" );
        auto b = Girl::make( "b" );
        auto c = Girl::make( "c" );
        auto d = a ;
        d->getProxy()->rebind(b);
        c->getProxy()->rebind(a);
        b->getProxy()->rebind(c);
        std::cout << "rebind succeeded." << std::endl;
    }
    std::cout << "Exited scope." << std::endl;
}

http://rextester.com/GXGNC55369

[cire]

I'm not sure exactly what's going wrong, but I suspect we've run afoul of undefined behavior somewhere. The following results in equivalent behavior with GCC (removing the virtual destructor from Common @ ideone.com will result in more interesting output.)

#include <iostream>
#include <memory>

struct Common { virtual ~Common() = default; };

struct B;
struct A : public Common
{
    std::shared_ptr<Common> _a;
    A() : _a(this){ std::cout << this << ": A()\n"; }
    ~A() { std::cout << this << ": ~A(), _a -> " << _a.get() << '\n'; }

    void rebind(const std::shared_ptr<Common>& c)
    {
        std::cout << this << ": rebind from " << _a.get() << " to " << c.get() << '\n';
        _a = c;
        std::cout << this << ": rebind complete.\n";
    }
};

struct B : public Common
{
    std::shared_ptr<Common> _b;
    B() : _b(this) { std::cout << this << ": B()\n"; }
    ~B() { std::cout << this << ": ~B(), _b -> " << _b.get() << '\n'; }

    void rebind(const std::shared_ptr<Common>& c)
    {
        std::cout << this << ": rebind from " << _b.get() << " to " << c.get() << '\n';
        _b = c;
        std::cout << this << ": rebind complete.\n";
    }
};

struct scope_guard
{
    scope_guard() { std::cout << "Scope entered\n"; }
    ~scope_guard() { std::cout << "Scope exited\n"; }
};


int main() 
{
    {
        scope_guard guard;

        A* a = new A;
        std::shared_ptr<B> b(new B);

        a->rebind(b);
    }

    {
        scope_guard guard;

        A* a = new A;
        std::shared_ptr<B> b(new B);

        b->rebind(std::shared_ptr<Common>(a));
    }
}

Edit: Ninja'd. Thanks for the input @JLBorges.

[JLBorges]

Missed this post earlier.

> GCC seems to think that the references are circular and so nothing is deleted.

Write a destructor with an observable side effect for Girl and GCC too will destroy the one and only Girl object during the rebind. What happens after that could vary wildly from implementation to implementation; we are in the realm of undefined behaviour.
clang++ with libc++ gives a backtrace for call to free with an invalid pointer (from the destructor of the weak pointer without an associated reached from an associated invalidated shared pointer)
http://coliru.stacked-crooked.com/a/5fb62f5918555ac4

This is fundamentally wrong:

A* a = new A() ;
std::shared_ptr<A> pa1(a) ;
std::shared_ptr<A> pa2(a) ;

[prestokeys]

JLBorges, I add to your solution

void Person::transform() {
	std::shared_ptr<Girl> newPerson = Girl::make("New Girl");
	getProxy()->rebind(newPerson);
}

and in main(), the line

Girl* girl = new Girl ("Girl");   girl->transform();

crashes because girl is not a shared_ptr (invalid use of shared_from_this()). This means that with your new design, all sub-objects of Person must be smart pointers (instead of regular pointers) everywhere in my program now, right? And all functions accepting Person pointers as argument must now have shared_ptr<Person> as argument? In other words, no more use of regular pointers for Person and its derived types anywhere?

Or I can still use regular pointers, but construct a shared_ptr from one just before calling a function like Person::transform()?

Girl* girl = new Girl ("Girl"); 
// Do whatever with girl ... and then finally the time comes to transform:
std::shared_ptr<Girl> temp = std::shared_ptr<Girl>(girl); 
temp->transform();

Just trying to anticipate the repurcussions of this new design.

[JLBorges]

> This means that with your new design, all sub-objects of Person must be
> smart pointers (instead of regular pointers) everywhere in my program now, right?

Yes, a complete objects of a derived type of person must be dynamically allocated and the object and its sub-objects must not be accessed through owning raw pointers.

Private or protected constructors with public static factory functions returning shared pointers can facilitate this usage.

As in the earlier snippet:

class Girl : public Person
{
    Girl( const std::string& name ) : Person( name ) { // private
        std::cout << "Girl::constructor: " << name << ' ' << this << '\n' ;
    }

    public:
        // enforce dynamic storage duration 
        static std::shared_ptr<Girl> make(const std::string& name) {
            return std::shared_ptr<Girl>( new Girl( name ) ) ;
        }

    // ...
 };

Note: technically, for an object that would never be accessed via a proxy, and for which shared_from_this() would never be invoked, this is not required.


> In other words, no more use of regular pointers for Person and its derived types anywhere.

Yes. To be precise, no owning raw pointers anywhere, and no multiple unrelated shared pointers to the same object.

[prestokeys]

JLBorges, I'm not sure but I think you misunderstood what the getProxy() is meant for in my program. Here is a test run:

#include <iostream>
#include <string>
#include <memory>

#define show(variable) std::cout << #variable << " = " << variable << std::endl;

class Person;  class PersonProxy;

class PersonInterface {
    public:
        virtual ~PersonInterface() = default;
        virtual std::shared_ptr<PersonProxy> getProxy() = 0;
};

class Person : public PersonInterface, public std::enable_shared_from_this<Person> {
	private:
        std::string name;
        std::shared_ptr<PersonProxy> proxy;  // Don't we have to keep this data member?
    protected:
        Person() = default;
        explicit Person (const std::string& n) : name(n) {}
    public:
        virtual std::shared_ptr<PersonProxy> getProxy() override ;
        std::string getName() const {return name;}
        void showProxy();
};

class PersonProxy : public PersonInterface, public std::enable_shared_from_this<PersonProxy> {
	private:
        std::shared_ptr<Person> actual;
        explicit PersonProxy( std::shared_ptr<Person> p ) : actual( p ) {}
    public:
        // enforce dynamic storage duration
        static std::shared_ptr<PersonProxy> make(std::shared_ptr<Person> p) {
            return std::shared_ptr<PersonProxy>( new PersonProxy(p) );
		}
        void rebind( std::shared_ptr<Person> p ) {
        	std::cout << "Before 'actual = p;' call: " << std::endl;  show(actual);  // test
		actual = p;
		std::cout << "After 'actual = p;' call: " << std::endl;  show(actual); // test
		}
        virtual std::shared_ptr<PersonProxy> getProxy() override { return actual->getProxy() ? actual->getProxy() : shared_from_this() ; }
        Person* getActual() const {return actual.get();}
		std::string getName() const {return actual->getName();}
};

class Girl : public Person {
    public:
	    Girl( const std::string& name ) : Person( name ) { 
 	       std::cout << "Girl::constructor: " << name << ' ' << this << '\n' ;
 	   }
        // enforce dynamic storage duration
        static std::shared_ptr<Girl> make(const std::string& name) {
            return std::shared_ptr<Girl>( new Girl( name ) ) ;
        }
        
        ~Girl() { 
            std::cout << "Girl::destructor: " << this << '\n' ; 
        }
};

std::shared_ptr<PersonProxy> Person::getProxy() {
    return PersonProxy::make( shared_from_this() ) ;
}

void Person::showProxy() {  // Test
	std::cout << "proxy->actual of " << getName() << " is " << getProxy()->getActual() << ", " << getProxy()->getName() << "." << std::endl;
}

int main() {
    {
        std::shared_ptr<Girl> a = Girl::make("a");
        std::shared_ptr<Girl> b = Girl::make("b");

	a->showProxy();
	a->getProxy()->rebind(b);
	a->showProxy();
    }
    std::cout << "Exited scope." << std::endl;
    std::cin.get();
}

Output:

Girl::constructor: a 0x3f1360
Girl::constructor: b 0x3f13b0
proxy->actual of a is 0x3f1360, a.
Before 'actual = p;' call:
actual = 0x3f1360  // correct
After 'actual = p;' call:
actual = 0x3f13b0  // correct
proxy->actual of a is 0x3f1360, a.  // No.  My intention is for a's new proxy->actual to be 0x3f13b0
Girl::destructor: 0x3f13b0
Girl::destructor: 0x3f1360
Exited scope.

I think the 'std::shared_ptr<PersonProxy> proxy;' data member is needed in Person.

[JLBorges]

> I think the 'std::shared_ptr<PersonProxy> proxy;' data member is needed in Person.

If all user access to an object of type person is through a person proxy, it is not required in the class person.

Cyclic references (a proxy trying to proxy for itself: eg. proxy1 => proxy2 => proxy3 => proxy1) will result in infinite loops; they have to be prevented.

This is one possible approach:

#include <iostream>
#include <string>
#include <memory>
#include <stdexcept>

struct person : std::enable_shared_from_this<person> {

    virtual ~person() = default ;
    virtual std::string name() const { return "'" + name_ + "'" ; }

    protected:
        person() = default;
        explicit person ( const std::string& name ) : name_(name) {}

    private: std::string name_ ;

    // non-copyable and non-moveable; always acces via references or (smart) pointers
    person( const person& ) = delete ;
    person& operator= ( const person& ) = delete ;
};

struct person_proxy : person {

    static std::shared_ptr<person_proxy> make( const std::shared_ptr<person>& p ) {
        return std::shared_ptr<person_proxy>( new person_proxy(p) );
    }

    std::shared_ptr<person_proxy> rebind( const std::shared_ptr<person>& p ) {

        show( std::cout << "before rebind: " << this << " => " );  // test

        if( is_cyclic(p) ) throw std::logic_error( "cyclic; can't be proxy for itself" ) ;

        actual = p;
        show( std::cout << "after rebind: " << this << " => " ) ;  // test

        return std::dynamic_pointer_cast<person_proxy>( shared_from_this() ) ;
    }

    virtual std::string name() const override { return actual->name(); }

    private:
        std::shared_ptr<person> actual;
        explicit person_proxy( const std::shared_ptr<person>& p ) : actual( p ) {}

        void show( std::ostream& stm ) {
            #ifndef NDEBUG
                stm << actual.get() ;
                auto p = std::dynamic_pointer_cast<person_proxy>(actual) ;
                if(p) p->show( stm << " => " ) ;
                else stm << '\n' ;
            #endif // NDEBUG
        }

        bool is_cyclic( const std::shared_ptr<person>& p ) const {
            auto pp = std::dynamic_pointer_cast<person_proxy>(p) ;
            if( pp && ( pp.get() == this ) ) return true ;
            else return pp ? is_cyclic( pp->actual ) : false ;
        }
};

struct girl : person {

    static std::shared_ptr<person_proxy> make( const std::string& name ) {
        return person_proxy::make( std::shared_ptr<girl>( new girl(name) ) ) ;
    }

    ~girl() {
        std::cout << "girl::destructor: " << girl::name() << ' ' << this << '\n' ;
    }

    private:
        girl( const std::string& name ) : person( name ) {
            std::cout << "girl::constructor: " << girl::name() << ' ' << this << '\n' ;
        }
};

struct catman : person {

    static std::shared_ptr<person_proxy> make( const std::string& name ) {
        return person_proxy::make( std::shared_ptr<catman>( new catman(name) ) ) ;
    }

    ~catman() {
        std::cout << "catman::destructor: " << catman::name() << ' ' << this << '\n' ;
    }

    private:
        catman( const std::string& name ) : person( name ) {
            std::cout << "catman::constructor: " << catman::name() << ' ' << this << '\n' ;
        }
};

int main() {

    auto a = girl::make( "alice" );
    std::cout << "+++ " << a->name() << " +++\n\n" ;

    auto b = catman::make( "the cheshire catman" );
    a->rebind(b) ;
    std::cout << "+++ " << a->name() << " +++\n\n" ;

    auto c = girl::make( "the queen of hearts" );
    auto d = a->rebind(c) ;
    std::cout << "+++ " << a->name() << " +++\n\n" ;

    try { d->rebind(a) ; } catch( const std::exception& e ) { std::cerr << "***** error: " << e.what() << '\n' ; }
    auto a1 = a ;
    try { c->rebind(a1) ; } catch( const std::exception& e ) { std::cerr << "***** error: " << e.what() << '\n' ; }

    std::cout << "+++ " << a->name() << " +++\n\n" ;
}

http://coliru.stacked-crooked.com/a/29fd22422a833fa2

[prestokeys]

Ok, using your new design idea, I have the following now, which works. But I just want your opinion. I mention my concern in the comments:

#include <iostream>
#include <string>
#include <memory>
#include <stdexcept>

struct PersonProxy;

struct State {
	virtual ~State() = default;
} *normal = new State;

class Charmed : public State {
	private:
		std::shared_ptr<PersonProxy> charmer;
	public:
		Charmed (std::shared_ptr<PersonProxy> p) : charmer(p) {}
		std::shared_ptr<PersonProxy> getCharmer() const {return charmer;}
};

struct Person : std::enable_shared_from_this<Person> {
    private:
		std::string name_ ;
    	State* state = normal;
    public:
    	virtual ~Person() = default ;
    	virtual std::string getName() const { return "'" + name_ + "'" ; }
    	virtual State* getState() const {return state;}
    	virtual void setState (State* s) {state = s;}
    	virtual void action(std::shared_ptr<PersonProxy>, std::shared_ptr<PersonProxy>){}
    	virtual bool isCharmed() const {return dynamic_cast<Charmed*>(state);}
    protected:
        Person() = default;
        explicit Person ( const std::string& name ) : name_(name) {}
	private:
	    Person( const Person& ) = delete ;
	    Person& operator= ( const Person& ) = delete ;
};

struct PersonProxy : Person {
	    static std::shared_ptr<PersonProxy> make( const std::shared_ptr<Person>& p ) {
	        return std::shared_ptr<PersonProxy>( new PersonProxy(p) );
	    }
	    std::shared_ptr<PersonProxy> rebind( const std::shared_ptr<Person>& p ) {
			actual = p;
	        return std::dynamic_pointer_cast<PersonProxy>( shared_from_this() ) ;
	    }
	    virtual std::string getName() const override { return actual->getName(); }
	    virtual State* getState() const override {return actual->getState();}
	    virtual void setState (State* s) override {actual->setState(s);}
	    virtual bool isCharmed() const override {return actual->isCharmed();}
		void action(std::shared_ptr<PersonProxy> p) {
			actual->action(std::dynamic_pointer_cast<PersonProxy>(shared_from_this()), p);
		}
    private:
        std::shared_ptr<Person> actual;
        explicit PersonProxy( const std::shared_ptr<Person>& p ) : actual( p ) {}
        bool is_cyclic( const std::shared_ptr<Person>& p ) const {
            auto pp = std::dynamic_pointer_cast<PersonProxy>(p) ;
            if( pp && ( pp.get() == this ) ) return true ;
            else return pp ? is_cyclic( pp->actual ) : false ;
        }
};

struct girl : Person {

    static std::shared_ptr<PersonProxy> make( const std::string& name ) {
        return PersonProxy::make( std::shared_ptr<girl>( new girl(name) ) ) ;
    }
    private:
        girl( const std::string& name ) : Person( name ) {}
        virtual void action(std::shared_ptr<PersonProxy>, std::shared_ptr<PersonProxy>) override {}
};

struct Wizard : Person {
    static std::shared_ptr<PersonProxy> make( const std::string& name ) {
        return PersonProxy::make( std::shared_ptr<Wizard>( new Wizard(name) ) ) ;
    }
    void castsCharmSpell (std::shared_ptr<PersonProxy> spellCaster, std::shared_ptr<PersonProxy> target) {
    	target->setState(new Charmed(spellCaster));
// std::shared_ptr<PersonProxy> spellCaster has to be passed because Person has no std::shared_ptr<PersonProxy> data member.
// This will have to be done with all functions like castsCharmSpell for derived types of Person.  Good design?
    }
    private:
        Wizard( const std::string& name ) : Person( name ) {}
        virtual void action(std::shared_ptr<PersonProxy> instigator, std::shared_ptr<PersonProxy> p) override {
			castsCharmSpell(instigator, p);
		}
};

int main() {
    std::shared_ptr<PersonProxy> a = girl::make( "Alice" );
    std::shared_ptr<PersonProxy> wizard = Wizard::make( "Wizard" );
	std::shared_ptr<PersonProxy> b = girl::make( "The queen of hearts" );

    wizard->action(a);
    if (a->isCharmed())
	std::cout << a->getName() << " is charmed by "  << dynamic_cast<Charmed*>(a->getState())->getCharmer()->getName() << "." << std::endl;
    
    wizard->rebind(b);
    std::cout << "'Wizard' has been transformed into " << wizard->getName() << "." << std::endl;
    if (a->isCharmed())
	std::cout << a->getName() << " is charmed by "  << dynamic_cast<Charmed*>(a->getState())->getCharmer()->getName() << "." << std::endl;
}

Output

'Alice' is charmed by 'Wizard'.
'Wizard' has been transformed into 'The queen of hearts'.
'Alice' is charmed by 'The queen of hearts'.

So there seems to be a trade-off. Go without the std::shared_ptr<PersonProxy> data member in Person and have EVERY function like castCharmSpell pass an extra std::shared_ptr<PersonProxy>, or have give Person a std::shared_ptr<PersonProxy> data member. Or is there a simpler way than the above that I'm missing?

[prestokeys]

Here is the above with a few lines added, whereby Person has a proxy data member. Which design is better?

#include <iostream>
#include <string>
#include <memory>
#include <stdexcept>

class PersonProxy;

struct State {
	virtual ~State() = default;
} *normal = new State;

class Charmed : public State {
	private:
		std::shared_ptr<PersonProxy> charmer;
	public:
		Charmed (std::shared_ptr<PersonProxy> p) : charmer(p) {}
		std::shared_ptr<PersonProxy> getCharmer() const {return charmer;}
};

class Person : public std::enable_shared_from_this<Person> {
    private:
	std::string name;
	std::shared_ptr<PersonProxy> proxy;  // ***added
    	State* state = normal;
    public:
    	virtual ~Person() = default ;
        void setProxy (std::shared_ptr<PersonProxy> p) {proxy = p;}
	  	virtual std::string getName() const { return "'" + name + "'" ; }
    	virtual State* getState() const {return state;}
    	virtual void setState (State* s) {state = s;}
    	virtual void action(std::shared_ptr<PersonProxy>) = 0;
    	virtual bool isCharmed() const {return dynamic_cast<Charmed*>(state);}
    protected:
        Person() = default;
        explicit Person ( const std::string& n) : name(n) {}
        std::shared_ptr<PersonProxy> getProxy() const {return proxy;}
 	private:
	    Person( const Person& ) = delete ;
	    Person& operator= ( const Person& ) = delete ;
};

class PersonProxy : public Person {
    private:
        std::shared_ptr<Person> actual;
	public:
	    static std::shared_ptr<PersonProxy> make( const std::shared_ptr<Person>& p ) {
	    	std::shared_ptr<PersonProxy> proxy = std::shared_ptr<PersonProxy>(new PersonProxy(p));
	    	p->setProxy(proxy);  // ***added
	    	return proxy;
	    }
	    std::shared_ptr<PersonProxy> rebind( const std::shared_ptr<Person>& p ) {
	    	if (is_cyclic(p)) throw std::logic_error ("Cyclic logic.  " + p->getName() + " cannot be proxy for itself" ) ;
			actual = p;
	        return std::dynamic_pointer_cast<PersonProxy>( shared_from_this() ) ;
	    }
	    virtual std::string getName() const override { return actual->getName(); }
	    virtual State* getState() const override {return actual->getState();}
	    virtual void setState (State* s) override {actual->setState(s);}
	    virtual bool isCharmed() const override {return actual->isCharmed();}
		virtual void action(std::shared_ptr<PersonProxy> p) override {actual->action(p);}
    private:
        explicit PersonProxy( const std::shared_ptr<Person>& p ) : actual( p ) {}
        bool is_cyclic( const std::shared_ptr<Person>& p ) const {
            auto pp = std::dynamic_pointer_cast<PersonProxy>(p) ;
            if( pp && ( pp.get() == this ) ) return true ;
            else return pp ? is_cyclic( pp->actual ) : false ;
        }
};

template <typename DERIVED>
class PersonCRTP : virtual public Person {
	public:
    	static std::shared_ptr<PersonProxy> make( const std::string& name ) {
    	    return PersonProxy::make( std::shared_ptr<DERIVED>( new DERIVED(name) ) ) ;
    	}
};

class Girl : public PersonCRTP<Girl> {
	public:
		Girl( const std::string& name ) : Person( name ) {}
    private:
        virtual void action(std::shared_ptr<PersonProxy>) override {}
};

class Wizard : public PersonCRTP<Wizard> {
    public:
 	   Wizard( const std::string& name ) : Person( name ) {}
 	   	void castsCharmSpell (std::shared_ptr<PersonProxy> target) {  // New form.  Only target needs to be passed.
			target->setState(new Charmed(getProxy()));  // simply pass its proxy data member
	}
    private:
        virtual void action(std::shared_ptr<PersonProxy> p) override {castsCharmSpell(p);}
};

int main() {
    std::shared_ptr<PersonProxy> a = Girl::make( "Alice" );
    std::shared_ptr<PersonProxy> wizard = Wizard::make( "Wizard" );
	std::shared_ptr<PersonProxy> b = Girl::make( "The queen of hearts" );

    wizard->action(a);
    if (a->isCharmed())
	std::cout << a->getName() << " is charmed by "  << dynamic_cast<Charmed*>(a->getState())->getCharmer()->getName() << "." << std::endl;
    
    wizard->rebind(b);
    std::cout << "'Wizard' has been transformed into " << wizard->getName() << "." << std::endl;
    if (a->isCharmed())
	std::cout << a->getName() << " is charmed by "  << dynamic_cast<Charmed*>(a->getState())->getCharmer()->getName() << "." << std::endl;
}

Output:

'Alice' is charmed by 'Wizard'.
'Wizard' has been transformed into 'The queen of hearts'.
'Alice' is charmed by 'The queen of hearts'.

[JLBorges]

> Which design is better?

I do not have a complete understanding of the design of your program; but here are a few thoughts:

1. There is nothing fundamentally wrong with a design where Person has a proxy data member. As long as recursive references are avoided. Go with this design if it caters to your requirements in a simpler way. (A classic envelope and letter idiom, where the envelope and the letter have the same polymorphic type.)

2. The problem in the design where Person does not have a proxy data member is that there may be multiple instances of std::shared_ptr<PersonProxy>, all leading to the same Person through different paths, and rebind() does not rebind all of them.

For instance:

#include <iostream>
#include <string>
#include <memory>
#include <stdexcept>

struct PersonProxy;

struct State {
    virtual ~State() = default;
} *normal = new State;

class Charmed : public State {
	private:
		std::shared_ptr<PersonProxy> charmer;
	public:
		Charmed (std::shared_ptr<PersonProxy> p) : charmer(p) {}
		std::shared_ptr<PersonProxy> getCharmer() const {return charmer;}
};

struct Person : std::enable_shared_from_this<Person> {
    private:
		std::string name_ ;
    	State* state = normal;
    public:
    	virtual ~Person() = default ;
    	virtual std::string getName() const { return "'" + name_ + "'" ; }
    	virtual State* getState() const {return state;}
    	virtual void setState (State* s) {state = s;}
    	virtual void action( /*std::shared_ptr<PersonProxy>,*/ std::shared_ptr<PersonProxy>){}
    	virtual bool isCharmed() const {return dynamic_cast<Charmed*>(state);}
    protected:
        Person() = default;
        explicit Person ( const std::string& name ) : name_(name) {}
	private:
	    Person( const Person& ) = delete ;
	    Person& operator= ( const Person& ) = delete ;
};

struct PersonProxy : Person {
	    static std::shared_ptr<PersonProxy> make( const std::shared_ptr<Person>& p ) {
	        return std::shared_ptr<PersonProxy>( new PersonProxy(p) );
	    }
	    std::shared_ptr<PersonProxy> rebind( const std::shared_ptr<Person>& p ) {
			actual = p;
	        return std::dynamic_pointer_cast<PersonProxy>( shared_from_this() ) ;
	    }
	    virtual std::string getName() const override { return actual->getName(); }
	    virtual State* getState() const override {return actual->getState();}
	    virtual void setState (State* s) override {actual->setState(s);}
	    virtual bool isCharmed() const override {return actual->isCharmed();}
		void action(std::shared_ptr<PersonProxy> p) {
			actual->action(/*std::dynamic_pointer_cast<PersonProxy>(shared_from_this()),*/ p); // **************
		}
    private:
        std::shared_ptr<Person> actual;
        explicit PersonProxy( const std::shared_ptr<Person>& p ) : actual( p ) {}
        bool is_cyclic( const std::shared_ptr<Person>& p ) const {
            auto pp = std::dynamic_pointer_cast<PersonProxy>(p) ;
            if( pp && ( pp.get() == this ) ) return true ;
            else return pp ? is_cyclic( pp->actual ) : false ;
        }
};

struct girl : Person {

    static std::shared_ptr<PersonProxy> make( const std::string& name ) {
        return PersonProxy::make( std::shared_ptr<girl>( new girl(name) ) ) ;
    }
    private:
        girl( const std::string& name ) : Person( name ) {}
        virtual void action( /*std::shared_ptr<PersonProxy>,*/ std::shared_ptr<PersonProxy>) override {} // **************
};

struct Wizard : Person {
    static std::shared_ptr<PersonProxy> make( const std::string& name ) {
        return PersonProxy::make( std::shared_ptr<Wizard>( new Wizard(name) ) ) ;
    }
    void castsCharmSpell ( /*std::shared_ptr<PersonProxy> spellCaster,*/ std::shared_ptr<PersonProxy> target) {
    	target->setState( new Charmed( /*spellCaster*/ PersonProxy::make( shared_from_this() ) ) ); // **************
// std::shared_ptr<PersonProxy> spellCaster has to be passed because Person has no std::shared_ptr<PersonProxy> data member.
// This will have to be done with all functions like castsCharmSpell for derived types of Person.  Good design?
    }
    private:
        Wizard( const std::string& name ) : Person( name ) {}
        virtual void action(/*std::shared_ptr<PersonProxy> instigator,*/ std::shared_ptr<PersonProxy> p) override { // **************
			castsCharmSpell(/*instigator,*/ p);
		}
};

int main() {
    std::shared_ptr<PersonProxy> a = girl::make( "Alice" );
    std::shared_ptr<PersonProxy> wizard = Wizard::make( "Wizard" );
	std::shared_ptr<PersonProxy> b = girl::make( "The queen of hearts" );

    wizard->action(a);
    if (a->isCharmed())
	std::cout << a->getName() << " is charmed by "  << dynamic_cast<Charmed*>(a->getState())->getCharmer()->getName() << "." << std::endl;

    wizard->rebind(b);
    dynamic_cast<Charmed*>(a->getState())->getCharmer()->rebind(b) ; // **************
    std::cout << "'Wizard' has been transformed into " << wizard->getName() << "." << std::endl;
    if (a->isCharmed())
	std::cout << a->getName() << " is charmed by "  << dynamic_cast<Charmed*>(a->getState())->getCharmer()->getName() << "." << std::endl;
}

http://coliru.stacked-crooked.com/a/2fa86d1a0e9a15d1