creating hash (or list) of member functions

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.

JonB

@Chris-Kawa
Thanks. You gotta love hardcore C++, it's so... simple and clean.

mzimmers

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

@Chris-Kawa
Thanks. You gotta love hardcore C++, it's so... simple and clean.

Now, now...no sarcasm.

But yeah...wouldn't you love to have today's compute resources available for solving the problems of 30 years ago?

J.Hilk

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

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

who says you can't ?

#include <array>

class SomeClass : public QObject
{
    Q_OBJECT
    typedef void (SomeClass::*SomeClassFunction)();

    std::array<SomeClassFunction,3> arrayOfSignalsPointers{&SomeClass::signal1,&SomeClass::signal2, &SomeClass::signal3};
    std::array<SomeClassFunction, 3> arrayOfSlotsPointers{&SomeClass::slot1, &SomeClass::slot2, &SomeClass::slot3};

public:
    explicit SomeClass(QObject *parent = nullptr) : QObject(parent)
    {
        QObject::connect(this, &SomeClass::signal1, this, &SomeClass::slot1);
        QObject::connect(this, &SomeClass::signal2, this, &SomeClass::slot2);
        QObject::connect(this, &SomeClass::signal3, this, &SomeClass::slot3);

        qDebug() << "Emit all signals";
        for(auto entry : arrayOfSignalsPointers){
            (this->*entry)();
        }

        qDebug() << "Call all slots directly";
        for(auto entry : arrayOfSlotsPointers){
            (this->*entry)();
        }
    }

signals:
    void signal1();
    void signal2();
    void signal3();

public slots:
    void slot1(){qDebug() <<  Q_FUNC_INFO;}
    void slot2(){qDebug() <<  Q_FUNC_INFO;}
    void slot3(){qDebug() <<  Q_FUNC_INFO;}
};

JonB

@J-Hilk said in creating hash (or list) of member functions:

typedef void (SomeClass::*SomeClassFunction)();

I said that you cannot use typedef void (*clientSlot)();, as the OP wrote and one would in C, to call a C++ member function. And you can't: as you show you need ClassName::*function not just plain *function.

Having said that, I was nonetheless not aware that you can use that to get a member function's address and then call (instance->*memberFunctionPointer)(). Thank you for clarifying.

So.... all this std::function<> and particularly std::bind() looks like the usual C++ "why would you want to write something simple when you can wrap it up to be complicated"? ;-)

Chris Kawa

@JonB said:

So.... all this std::function<> and particularly std::bind() looks like the usual C++ "why would you want to write something simple when you can wrap it up to be complicated"? ;-)

No, it's a way to be generic. To write typedef void (SomeClass::*SomeClassFunction)() you have to hardcode SomeClass i.e. know it up front. Notice that what @J-Hilk posted will work with one particular class only. I know he just shows how to call a member function from a pointer and that's fine, but it doesn't do much for the original problem.
std::function doesn't care. It just takes any functor you give it and std::bind creates a functor from anything callable you give it.