20

I want to be able to call functions based on the data I read from file. So for each item type, I want to call the desired reader method. I wrote this code, but it does not compile where I want to add function pointers to the map. What is wrong?

#include <vector>
#include <map>
#include <iostream>
class reader
{

  std::map< std::string, void(*)()> functionCallMap; // function pointer
  void readA(){ std::cout << "reading A\n";};
  void readB(){ std::cout << "reading B\n";};;
public:
  reader()
  {
    *functionCallMap["A"] = &reader::readA;*
    *functionCallMap["B"] = &reader::readB;*
  }

  void read()
  {
   auto (*f) = functionCallMap["A"];
   (*f)();
  }



};

I am filling the map at Constructor.

Josh Darnell
  • 11,304
  • 9
  • 38
  • 66
Ring Zero.
  • 459
  • 3
  • 12
  • 6
    A pointer to a non-member function is *not* the same as a pointer to a member function. The big difference is that member functions needs objects to be called on. You can solve it by using [`std::function`](https://en.cppreference.com/w/cpp/utility/functional/function) instead, together with either [lambda expressions](https://en.cppreference.com/w/cpp/language/lambda) or [`std::bind`](https://en.cppreference.com/w/cpp/utility/functional/bind). – Some programmer dude Nov 16 '18 at 11:25

4 Answers4

25

You can use std::function with a lambda or std::bind :

class reader
{
    std::map<std::string, std::function<void()>> functionCallMap;

    void readA() { std::cout << "reading A\n"; };
    void readB() { std::cout << "reading B\n"; };

public:
    reader()
    {
        functionCallMap["A"] = [this]() { readA(); };
        functionCallMap["B"] = std::bind(&reader::readB, this);
    }

    void read()
    {
        functionCallMap["A"]();
        functionCallMap["B"]();
    }
};
Siliace
  • 652
  • 5
  • 13
  • 2
    I prefer your solution because it's more C++ish and cleaner than raw function pointer. Anyway lamba should be preferred to std::bind (Scott Meyer's Effective Modern C++) – Moia Nov 16 '18 at 11:46
  • 2
    Yes, It is a nice solution. But, according to Jason Turner, bind is expensive both in compile time and memory usage. – Ring Zero. Nov 16 '18 at 17:45
  • @RingZero You should still use std::function instead of a function pointer, I would strongly advise switching the marked answer to this one for future readers. Using actual function pointers is bad for a number of reasons, but one issue is that you can't bind to anything [*that is a lambda pointer with variable capture*](https://stackoverflow.com/questions/28746744/passing-capturing-lambda-as-function-pointer). You will notice that std::function doesn't need the class pointer in the signature using the above. – Krupip Nov 16 '18 at 21:58
18

You need to use pointers to member functions, like this:

class reader
{
    using FuncPtr = void(reader::*)(); // function pointer
    std::map< std::string, FuncPtr> functionCallMap;
    void readA(){ std::cout << "reading A\n"; }
    void readB(){ std::cout << "reading B\n"; }
public:
    reader()
    {
        functionCallMap["A"] = &reader::readA;
        functionCallMap["B"] = &reader::readB;
    }

    void read()
    {
        auto f = functionCallMap["A"];
        (this->*f)();
    }
};

int main()
{
    reader r;
    r.read();
}
snake_style
  • 1,139
  • 7
  • 16
9

There are two answers so far, this and this.

The obvious difference is that one uses std::function and other uses function pointers. This is not the important difference!!

The key point is that the member functions are non-static member functions. So, they are not of type void().

They are of type void(reader::*)(). Thus, they can be only called if an object of type is reader is given; one can understand this somewhat as a hidden parameter.

The first answer just fixes the problem by specifying the correct type. This can be done using function pointers (as presented) or using std::function (The latter is much more expensive!).

The second answer fixes the problem by binding the function pointer to the particular instance of the class. After binding, the type is then indeed void(). This cannot be done using raw function pointers (because they can only point to a function and not an object/function pair!).

pushkin
  • 9,575
  • 15
  • 51
  • 95
Handy999
  • 766
  • 4
  • 8
  • Also note that the timing of the binding is a design choice. I.E. does the application always want to bind the object storing the function pointers or potentially another one? – Keith Nov 16 '18 at 21:15
1

I ended up with this solution. It does the job, but I have some doubts over its aesthetics. Anyway, to sum up, I ended up with this code:

#include <map>
#include <iostream>
#include <functional>
class reader
{
    std::map< std::string, std::function<void(std::string tableName)>> functionCallMap; // function pointer
    void readA(const std::string tableName){ std::cout << "reading:" << tableName<< "\n"; }
    void readB(const std::string tableName){ std::cout << "reading:" << tableName <<"\n"; }
public:
    reader()
    {
      functionCallMap["A"] = std::bind(&reader::readA, this, std::placeholders::_1);
      functionCallMap["B"] = std::bind(&reader::readA, this, std::placeholders::_1);
    }

    void read()
    {
      const std::string table_name = "A";
      functionCallMap[table_name](table_name);
    }
};

int main()
{
    reader r;
    r.read();
}

I pass the table name to the reader, it is nicely done with the bind and placeholder.

Ring Zero.
  • 459
  • 3
  • 12
  • You should use string references in your callback functions instead of a copy. And as Moia said, you really should use a lambda instead of `std::bind`. About the aesthetics, I don't understand why you need a map of function since `readA` and `readB` are the same. Moreover `readB` is never used so why don't just put so code of `readA` into `read`? – Siliace Nov 18 '18 at 14:14
  • @Siliace, It seems lambda and bind has its own pros and cons, read A and read B are going to read different kinds of tables, different formats and invoke different factory methods down the line. – Ring Zero. Nov 18 '18 at 14:56