Non-static function as parameter to another function in C++

Question

I've made a class student with subclass comparator. Constructor of comparator takes one argument called cmp_mode that specifies how we should compare students.

class student
{
public:
    std::string name;
    int course, group;

    student(std::string name,
            int course,
            int group): name(name)
    {
        this->course = course;
        this->group = group;
    }

    enum cmp_mode
    {
        NAME,
        COURSE,
        GROUP
    };

    class comparator
    {
        cmp_mode mode;

    public:
        comparator(cmp_mode mode)
        {
            this->mode = mode;
        }

        bool compare(student s1, student s2)
        {
            if (mode == NAME)
                return s1.name < s2.name;
            else if (mode == COURSE)
                return s1.course < s2.course;
            else if (mode == GROUP)
                return s1.group < s2.group;
            else
                throw "Oh god! We can't compare variables using these keys!";
        }
    };

};

Also, I've created a list of students and now I want to sort this list using comparator subclass.

std::list<student> l;

student st1("Anya", 2, 5);
student st2("Misha", 4, 2);
student st3("Yasha", 1, 3);

l.push_back(st1);
l.push_back(st2);
l.push_back(st3); 

student::comparator by_group(student::GROUP);

l.sort(by_group.compare);

But I'm getting the following error.

ERROR: Reference to non-static member function must be called.

So what should I do? How can I adjust sorting in the better way?

@juanchopanza sorry. I've just added the line with the definition. — Tikhon Belousko
– Tikhon Belousko, Commented Sep 30, 2012 at 7:07
Why not just rename compare to operator() const and pass the comparator object directly? That's how the standard comparators (e.g. less) work. — Philipp
– Philipp, Commented Sep 30, 2012 at 10:41

PiotrNycz · Accepted Answer · 2012-09-30 10:31:21Z

3

I'm starting from your comment:

I thought that I can write compare function for each case, but it seems even worse.

Why worse? Personally I think it is faster to run (at least faster to compile) and easier to maintain (shorter functions).

Isn't this much simpler to write:

l.sort(student::compare_by_group);

And this implementation easier to maintain:

class student
{
...    
    static bool compare_by_name(const student& s1, const student& s2)
    {
        return s1.name < s2.name;
    }
    static bool compare_by_course(const student& s1, const student& s2)
    {
        return s1.course < s2.course;
    }
    static bool compare_by_group(const student& s1, const student& s2)
    {
        return s1.group < s2.group;
    }

    // Add the followings only if you really need this stuff 
    // e.g. to get comparator type from elsewhere
    enum cmp_mode
    {
        NAME,
        COURSE,
        GROUP
    };

    static bool compare(cmp_mode, const student& s1, const student& s2)
    {
        switch(cmp_mode) {
          case NAME: return compare_by_name(s1, s2);
          ...
        }
    } 

};

answered Sep 30, 2012 at 10:31

PiotrNycz

24.9k10 gold badges77 silver badges122 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Tikhon Belousko Over a year ago

Yes, I see that writing each function is very fast, but I needed something more flexible. It's a specification of my task.

Philipp · Accepted Answer · 2012-09-30 10:49:43Z

2

Like Piotr, I'd recommend to write comparators for each property, which is faster and less error-prone. However, I'd recommend to use functor objects instead of the static functions:

struct compare_by_name {
  bool operator()(const student& a, const student& b) const {
    return a.name < b.name;
  }
};

If you need the comparators only once or maybe twice and you're using C++11, prefer an inline lambda:

l.sort([](const student& a, const student& b){ return a.name < b.name; });

If you absolutely need a dynamic comparator, write it as normal functor object, i.e., define an operator():

bool operator()(const student& a, const student& b) const {
  switch (mode) {
    case NAME:
      return a.name < b.name;
    // No default case, the constructor (if at all) should check whether the mode
    // is valid.
  }
}

answered Sep 30, 2012 at 10:49

Philipp

50.1k12 gold badges88 silver badges112 bronze badges

4 Comments

PiotrNycz Over a year ago

+1 for inline lambda. Do you know any advantages of stateless functors over functions?

Philipp Over a year ago

@PiotrNycz: In the past, compilers were able to inline them, unlike function pointers. Not sure whether that is still an issue. If stored somewhere (e.g. in a std::set), they can be made to take up zero space.

PiotrNycz Over a year ago

The first is probably not true today, unless they have to be stored as function pointers. And the second is true - I forget about this - thanks for this clarification, very helpful. However I can imagine implementation of std::set where when passing function pointer it is stored functor calling this function not function itself - but this would only complicate things...

Tikhon Belousko Over a year ago

@Philipp It's a very nice decision to define operator() it's exactly what I need. Thanks!

juanchopanza · Accepted Answer · 2012-09-30 07:22:11Z

2

Non-static member functions have an implicit parameter of the type of which the function is a member. They need an instance of this type in order to work. You can use std::bind (or boost.bind if you don't have C++11 support) to "bind" a student::comparator object to the first parameter of the comparison function:

student::comparator by_group(student::GROUP);
using namespace std::placeholders;
auto comp = std::bind(student::comparator::compare, &by_group, _1, _2);
l.sort(comp);

In the code above, the by_group object is bound as first argument to the student::comparator::compare function, and the resulting function object takes two student objects and returns a bool:

std::cout << std::boolalpha;
const student& s1 = l[0];
const student& s2 = l[1];
std::cout << comp(s1, s2) << "\n"; // prints "true" or "false".

I would also advise you to change the signature of the comparison member function to

bool compare(const student& s1, const student& s2) const;

There is no reason to pass by value, and there is no reason for the member function not to be const.

edited Sep 30, 2012 at 7:22

answered Sep 30, 2012 at 7:10

juanchopanza

228k35 gold badges421 silver badges493 bronze badges

3 Comments

Tikhon Belousko Over a year ago

Is it a good decision to use class comparator? Or is there another way to adjust sorting? I just don't have any other ideas. I thought that I can write compare function for each case, but it seems even worse.

juanchopanza Over a year ago

I think it is a good approach. If you have std::function you can simplify it by replacing the comparator_by_group class with a static function taking student::cmpmode, const student&, const student&, and bund the 1st parameter to a specific enum value. But that is just a detail.

PiotrNycz Over a year ago

@DaZzz see my answer. Frankly I always believe in simplicity,,,

Collectives™ on Stack Overflow

Non-static function as parameter to another function in C++

3 Answers 3

1 Comment

4 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

4 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related