Dealing with circular includes in the context of circular references

Question

As a follow up to an older question of mine, I wish to implement a client-server mockup simulation, where the client initiates a sequence of actions that involve calling methods on the server, which, in turn, can call methods on the client (let's ignore the issue that the stack may blow up).

More specifically, since I want to split the implementation from the definition, I will have server.h and server.cpp for the Server class and client.h and client.cpp for the Client class. Since Server holds a reference to Client and calls methods from it, it needs to #include "client.h". Also, Client holds a reference to Server and calls methods from it, it needs to #include "server.h". At this point, even if I use header guards in both server.h and client.h, it still messes up (yeah, it's expected) so I decided to forward-declare the Server class in client.h and the Client class in server.h. Unfortunately, this is not enough to solve the issue, because I'm also calling methods from the two classes, so I managed to make it compile & run (properly, as far as I can tell), by including server.h in client.cpp and client.h in server.cpp.

Does the above "hack" sound reasonable? Should I expect some unforeseen consequences? Is there any "smarter" way to do this without having to implement a proxy class?

Here's a rudimentary sample of how the implementation will look like:

file client.h:

#ifndef CLIENT_H
#define CLIENT_H

#include <iostream>
#include <memory>

class Server;

class Client
{
private:
    std::shared_ptr<const Server> server;

public:
    Client () {}

    void setServer (const std::shared_ptr<const Server> &server);

    void doStuff () const;

    void doOtherStuff () const;
};

#endif

file client.cpp:

#include "client.h"
#include "server.h"

void Client::setServer (const std::shared_ptr<const Server> &server)
{
    this->server = server;
}

void Client::doStuff () const
{
    this->server->doStuff();
}

void Client::doOtherStuff () const
{
    std::cout << "All done!" << std::endl;
}

file server.h:

#ifndef SERVER_H
#define SERVER_H

#include <iostream>
#include <memory>

class Client;

class Server
{
private:
    std::weak_ptr<const Client> client;

public:
    Server () {}

    void setClient (const std::weak_ptr<const Client> &client);

    void doStuff () const;
};

#endif

file sever.cpp:

#include "server.h"
#include "client.h"

void Server::setClient (const std::weak_ptr<const Client> &client)
{
    this->client = client;
}

void Server::doStuff () const
{
    this->client.lock()->doOtherStuff();
}

file main.cpp:

#include <iostream>
#include <memory>

#include "client.h"
#include "server.h"

int main ()
{
    std::shared_ptr<Client> client(new Client);
    std::shared_ptr<Server> server(new Server);

    client->setServer(server);
    server->setClient(client);

    client->doStuff();

    return 0;
}

Answer 1

That looks good to me. Forward declaring server in the client.h and forward declaring client in the server.h is the right thing to do.

It is perfectly fine to then include both header files in the .c or .cpp file - all you need to avoid is including the header files in a circle.

Answer 2

Does the above "hack" sound reasonable? Should I expect some unforeseen consequences? Is there any "smarter" way to do this without having to implement a proxy class?

Forward declaration and use include directive to is the normal and right way to break circular include.

Answer 3

The "Hack" is none, it's perfectly common practice to separate declaration and implementation of the two classes as you did. And it's perfectly normal that the *.cpp include both Headers.

---

Sidenote: First consider different signatures for your setServer and setClient methods: In both methods, you copy the argument. Both copies are nontrivial, since the use_counts and/or weak_count have to be updated. If the argument indeed is an existing argument, that is ok, but if it is a temporary, the copy will increase the count and destruction of the temporary will decrease it again, each time an internal pointer has to be dereferenced. In contrast, moving a shared_ptr or weak_ptr does not affect the use counts but resets the temporary. Destruction of that reset temporary again does not affect the use count (it effectively is a null pointer). Secondly, always prefer make_shared over simple new, because it saves you one allocation. So use this implementation instead:

void Client::setServer (std::shared_ptr<const Server> server)
{
    this->server = std::move(server);
}

int main ()
{
    auto client = std::make_shared<Client>(); //prefer make_shared
    auto server = std::make_shared<Server>();
    /* 1 */
    client->setServer(server); //by copy, if you need to continue to use server
    /* 2 */
    server->setClient(std::move(client)); //by moving
}

Call 1 will be as expensive at it was, you make one copy pf the shared_ptr, only this time you make it while passing the argument, not inside the method. Call 2 will be cheaper, because the shared_ptr is moved around and never copied.

---

My following statement is false (see comments) and applies only to unique_ptr, not to shared_ptr:

But: Since you use a std::shared_ptr<const Server> in Client, you will have to define Client's destructor inside client.cpp. The reason is that if you don't, the compiler will generate it for you, calling the shared_ptr's and thus Server's destructor which has not been declared inside client.h. At reasonably high warning levels you compiler should complain about calling delete on a an undefined class' pointer.