creating hash (or list) of member functions

mzimmers

Hi all -

I have a QTimer that expires periodically. Upon expiration, it will read a member of a QHash that contains the object to notify, and its slot (expressed as a function pointer). This is what I'm trying:

typedef void (*clientSlot)();
class Timer : public QObject
{
    Q_OBJECT
public:
    void registerClient(QObject *client, clientSlot cs);
...
}
class Clock : public QObject
{
    Q_OBJECT
public:
    void update();
...
}
int main(int argc, char *argv[]) {
    Timer timer;
    Clock *clock = new Clock();
    timer.registerClient(clock, &Clock::update);
...
}

but I'm getting an error on the registerClient line:


Cannot initialize a parameter of type 'clientSlot' (aka 'void (*)()') with an rvalue of type 'void (Clock::*)()'
timer.h: passing argument to parameter 'cs' here

I'm not sure how to interpret this error message. I'd have assumed that the address to a function would be equivalent to a function pointer, but...perhaps not.

Can someone advise me on what I'm doing wrong here?

Thanks...

JonB

@mzimmers said in creating hash of slots:

(aka 'void (*)()') with an rvalue of type 'void (Clock::*)()'

Your void (*clientSlot)() describes a free or global C-type/C++ :: function. You cannot use that for a C++ class member function

I'll leave the C++ experts to say what you would need, and probably not to do something like this :)

Christian Ehrlicher

An example is for instance QWidget:addAction(const QString &text, const QObject *receiver, const char *member, Qt::ConnectionType type = Qt::AutoConnection).

Chris Kawa

There's multiple ways you can do it. If you go the slot name route that Christian mentioned it could look something like this:

class Timer : public QObject
{
    Q_OBJECT

    QHash<QObject*, QByteArray> callbacks;
public:
    void registerClient(QObject* client, const char* slot)
    {
        // that's how you store it;
        callbacks[client] = slot;

        // that's how you invoke it
        QMetaObject::invokeMethod(client, callbacks[client].data());
    }
};

The good thing is that you get a solid meta call, so all the usual thread safety of connections and whatnot.
The bad thing is that you would register it like this timer.registerClient(clock, "update");, which means it's prone to typos and refactor bugs, as it's a string and the compiler can't help you. The method also needs to be meta invokable, so either marked as slot or Q_INVOKABLE macro.

Another way you can tackle it is using templates and std::function.

class Timer : public QObject
{
    Q_OBJECT

    QHash<QObject*, std::function<void()>> callbacks;
public:
    template<typename T>
    void registerClient(T* client, void(T::*cs)())
    {
        // that's how you store it
        callbacks[client] = std::bind(cs, client);

        // that's how you invoke it
        callbacks[client]();
    }
};

The good thing is it's compile time checked and there are no strings involved. It can also be any class method, not just slots or meta invokables.
The bad thing is that it's just an old regular function call, so you don't get all the nice safety guarantees of meta calls e.g. automatically scheduling the call on the object's thread.

These are just two examples, but there's more ways you can do it to get all the benefits and none of the drawbacks. It's much more labor intensive though. You can take a look at how QObject::connect does it. Quite a lot of magic involved.

mzimmers

@Chris-Kawa thanks for the detailed reply. I prefer your 2nd example, but I'm getting a linker error:

undefined reference to `void Timer::registerClient<Clock>(Clock*, void (Clock::*)())'

I'm not sure how <Clock> got into the above; do I need to cast my clock object to a generic QObject for the call?

Thanks...

mzimmers

In case anyone's interested, I decided to simplify the problem a bit, by allowing the timer to assume the client method name would always be the same. This allowed me to convert the QHash to a QList, and the code reads a little easier (to me, at least). Here's a semi-complete implementation of what Chris explained above (2nd example); let me know if you see any problems with it:

const int TIMER_INTERVAL_MS = 1000;
class Timer : public QObject
{
    Q_OBJECT
private:
    QTimer m_timer;
    QList<QObject *> m_clients;
public:
    explicit Timer(QObject *parent = nullptr);
    void startTimer() { m_timer.start(TIMER_INTERVAL_MS); }
    void registerClient(QObject *client) {
    if (!m_clients.contains(client)) {
        m_clients.append(client);
    }
}

public slots:
    void notifyClient() {
    static QList<QObject *>::iterator it = m_clients.begin();
    QMetaObject::invokeMethod(*it, "update"); // hardcoded for simplicity.

    // advance the iterator.
    it++;
    if (it == m_clients.end()) {
        it = m_clients.begin();
    }
}
class Client : public QObject
{
    Q_OBJECT
    QString m_name;
public:
    explicit Client(QObject *parent = nullptr, QString name = "") : QObject{parent}, m_name(name)
    {}

public slots:
    void update() { qDebug() << m_name << "updated"; }
};

int main(int argc, char *argv[]) {
    QCoreApplication a(argc, argv);

    Client client1 (&a, "client1");
    Client client2 (&a, "client2");
    Client client3 (&a, "client3");
    Timer timer;

    timer.registerClient(&client1);
    timer.registerClient(&client2);
    timer.registerClient(&client3);

    timer.startTimer();

    return a.exec();
}

Chris Kawa

@mzimmers said in creating hash of slots:

do I need to cast my clock object to a generic QObject for the call?

No, you just call it like in your original post: timer.registerClient(clock, &Clock::update);.
Linker error would suggest you don't have the function definition, but it's a template, so it should be right there with the declaration. Did you maybe implement it in the cpp file? Template body needs to be in the header with the declaration.

JonB

@mzimmers said in creating hash of slots:

void notifyClient() {
static QList<QObject *>::iterator it = m_clients.begin();
it++

Personally I would not do this with that static. You may not be intending to do this, but try:

    Client client1 (&a, "client1");
    Client client2 (&a, "client2");
    Timer timer, timer2;

    timer.registerClient(&client1);
    timer2.registerClient(&client2);

    timer.startTimer();
    timer2.startTimer();

You don't show where you call notifyClient() from, but I assume it's connected as the slot for m_timer.timeout() signal. With the above code I would expect it never to do the "invokeMethod()" stuff on client2 (whichever client is not the first one where notifyClient() is called), only ever on client1 (and to do it on that twice as often). And if you deleted timer/client1 it should "crash" when timeout next ticks on timer2/client2.

I would not make my QList<QObject *>::iterator it member variable static.

Chris Kawa

@mzimmers said:

let me know if you see any problems with it

Iterators of QList can become invalid after container modification (insertion/removal of elements), so saving it in the static is a bug.

Hardcoding slot name in the function is not very elegant. I wouldn't recommend that.

Don't do QString name = "". If you want an empty string use the default constructor QString name = QString(). It saves on unnecessary allocation.

mzimmers

@Chris-Kawa @JonB OK, so an iterator is a bad idea here. So, I guess I need a member index instead, and I'll just manually keep track of where in the list I am?

EDIT:

I'm back to implementing Chris' first suggestion, but I need a way to go through the hash in a round-robin manner. If an iterator isn't recommended, how do I do this? Maybe a QHash isn't the right container if all the values are the same.

In other words:

void Timer::notifyClient()
{
    QObject *client; // how do I know which QHash element to use?
    QMetaObject::invokeMethod(client, m_clients[client].data());
}

Thanks...

Chris Kawa

@mzimmers Sorry, it's not clear to me what you want to achieve. The way you have it set up you can have multiple clients registered for each timer separately. When a single timer times out do you want to notify all clients of all timers, all clients of one timer, one client from all the timers or one client from one timer?

You can't use any statics if you want to handle each timer separately, as static variables are shared between instances. If you want to notify one client for one timer going through the list each time out then don't use hash. Put clients in a list and keep an index. Just make sure you don't go out of range if a client unregisters and the list shrinks.

mzimmers

@Chris-Kawa the plan was to have a list of items that require periodic updates. The Timer class would keep a list of these items, and upon timer expiry, would notify one of the items in the list. The intention was to avoid doing all of the updates at once (to spread out the resource utilization).

If I use a list, what does the call to the update method look like? I'm having trouble properly forming it. I'm guessing that I can't just keep a list of the callbacks, right - I need the object as well?

Chris Kawa

@mzimmers said:

If I use a list, what does the call to the update method look like?

For example like this:

class Timer : public QObject
{
    Q_OBJECT

    QList<std::function<void()>> clients;
    int client_index = -1;

public:
    template<typename T>
    void registerClient(T* client, void(T::*cs)())
    {
        clients.push_back(std::bind(cs, client));
    }

    void notifyTimer()
    {
        if (!clients.isEmpty())
        {
            if (++client_index >= clients.size())
                client_index = 0;

            clients[client_index]();
        }
    }
};

and you register clients like this:

timer.registerClient(clock1, &Clock::update);
timer.registerClient(clock2, &Clock::update);
...

mzimmers

@Chris-Kawa yeah, I'd actually gotten something working, but I like yours better, because it accepts the callback as an argument, rather than hardcoding it in the timer.

I think this topic is closed, but I wonder if you could give me an explanation for the use of the std::bind. The list is just a list of QObjects; how does this "attach" the callback function?

Thanks!

Chris Kawa

@mzimmers In my example it's not a list of QObjects. It's a list of std::function objects.
Clock::update() is a class member function, so to call it you need an instance of that class i.e. instance->update(). The way class member functions work is that they really are just regular functions that have a hidden implicit this parameter, so in effect it's like Clock::update(instance).
std::bind, as the name suggests, creates a callable object that binds a functor with a parameter, so you can call it as if there was no parameter.
The way to think about it is that std::bind(cs, client) creates a struct with the operator(), something like this:

struct Something
{
   Clock* client;
   void operator()() { client->update(); }
}

so it turns a class member function with hidden this parameter into something that can be called without parameters. Then I just store it in a std::function object that can hold any type of callables (functions, functors, lambdas etc.).
In other words std::bind creates something that holds information about both object and a function pointer, so you don't need anything extra to call it.

JonB

@mzimmers said in creating hash (or list) of member functions:

but I wonder if you could give me an explanation for the use of the std::bind. The list is just a list of QObjects; how does this "attach" the callback function?

All of this in place of the typedef void (*clientSlot)(), with C life used to be so simple :) We can't use that to call a C++ class member function on an instance. So...

std::function<void()>

I can be used to call a C++ class member method.

registerClient(T* client, void(T::*cs)())

Here's my client object (of a certain type), and here is the class member function.

clients.push_back(std::bind(cs, client));

Creates and pushes an object which, when invoked, will call cs(client). Which turns out to be the same as client->cs(). Which I am just about to question @Chris-Kawa on...!

JonB

@Chris-Kawa said in creating hash (or list) of member functions:

The way class member functions work is that they really are just regular functions that have a hidden implicit this parameter, so in effect it's like Clock::update(instance).

OMG! But where does C++ tell you this and that you can write code to use it? I had no idea this was "documented" or "supported". I assumed implementation was opaque/abstract.

Chris Kawa

@JonB said:

But where does C++ tell you this and that you can write code to use it?

Well no, you can't currently write it like that. I meant it conceptually. That's just what the compiler does anyway (you can see it e.g. in the mangled function signatures when inspecting C++ library exports).

Although the so called Uniform Call Syntax has been proposed multiple times over the years, including by Mr. C++ himself: N4474, so you might see it in some future standard version.

JonB

@Chris-Kawa said in creating hash (or list) of member functions:

Well no, you can't currently write it like that. I meant it conceptually.

Oh, right! For a while there I thought you were saying literally.

I suppose I ought go look at what magic std::bind() actually does, then it would be clear. But I just know it's going to look complicated.... :(

Chris Kawa

@JonB said:

But I just know it's going to look complicated.... :(

It does look a bit complicated, but it has to deal with variable number of perfectly forwarded arguments and a lot of weird corner cases users come up with. Also it's the standard library, so it's mangled with all those underscore names and defensive programming style, but if you squint a little you'll see it basically returns a class with operator() like I mentioned.