list::sort comparator help

[JDong]

Hi everyone,

I am having trouble with sorting lists. I read the article on this website about list::sort but I can't get it to compile as I wish.

Here is my comparator:

bool HarvestManager::compare_byDistance (std::pair<Unit*, std::pair<Unit*, Unit*>> mineralGroup1, std::pair<Unit*, std::pair<Unit*, Unit*>> mineralGroup2)
{ 
//Implementation not shown
}

It is declared in the header as:

bool compare_byDistance (std::pair<BWAPI::Unit*, std::pair<BWAPI::Unit*, BWAPI::Unit*>> mineralGroup1, std::pair<BWAPI::Unit*, std::pair<BWAPI::Unit*, BWAPI::Unit*>> mineralGroup2);

I tried using it to sort my list but it always gives me the two errors:

error C3867: 'HarvestManager::compare_byDistance': function call missing argument list; use '&HarvestManager::compare_byDistance' to create a pointer to member
error C2660: 'std::list<_Ty>::sort' : function does not take 1 arguments
with
[
_Ty=std::pair<BWAPI::Unit *,std::pair<BWAPI::Unit *,BWAPI::Unit *>>
]

When I try to do this:

mineralGroups.sort(compare_byDistance);

mineralGroups is in the header as:

private:
	std::list<std::pair<BWAPI::Unit*, std::pair<BWAPI::Unit*, BWAPI::Unit*>>> mineralGroups;
        //other stuff 

[m4ster r0shi]

When I saw your username and the BWAPI stuff in your post, I thought you were Jaedong...

On topic, try making your comparator a static function:

#include <iostream>
#include <list>

struct My
{
    static bool IntComparator    (int    n1, int    n2);
    static bool DoubleComparator (double d1, double d2);
};

bool My::IntComparator    (int    n1, int    n2) { return n1 > n2; }
bool My::DoubleComparator (double d1, double d2) { return d1 > d2; }

template <class Container>
std::ostream & print(const Container & c)
{
    typename Container::const_iterator cur_it = c.begin();
    typename Container::const_iterator end_it = c.end();

    for (; cur_it != end_it; ++cur_it)
        std::cout << *cur_it << " ";

    return std::cout;
}

int main()
{
    std::list<int>    int_list;
    std::list<double> double_list;

    int_list.push_back(2);
    int_list.push_back(3);
    int_list.push_back(1);

    double_list.push_back(2.2);
    double_list.push_back(3.3);
    double_list.push_back(1.1);

    std::cout << "unsorted:\n";

    print(int_list)    << std::endl;
    print(double_list) << std::endl;

    int_list.sort();
    double_list.sort();

    std::cout << "\nsorted using default comparator:\n";

    print(int_list)    << std::endl;
    print(double_list) << std::endl;

    int_list.sort    (My::IntComparator);
    double_list.sort (My::DoubleComparator);

    std::cout << "\nsorted using custom comparator:\n";

    print(int_list)    << std::endl;
    print(double_list) << std::endl;

    return 0;
}

Also, since the things you sort are not primitives, you should pass them by const reference in your comparator:

bool HarvestManager::compare_byDistance (
    const std::pair<Unit*, std::pair<Unit*, Unit*>> & mineralGroup1, 
    const std::pair<Unit*, std::pair<Unit*, Unit*>> & mineralGroup2
) { /* ... */ }

[JDong]

Hmm wow I think that worked, the errors are resolved.

Could you elaborate though, on why the comparator should be static?

[m4ster r0shi]

JDong wrote:

Could you elaborate though, on why the comparator should be static?

Yes, take a look at this here:

#include <iostream>

using namespace std;

template <class Predicate>
void print_if(int * array, unsigned size, Predicate pred)
{
    for (unsigned i = 0; i < size; ++i)
        if ( pred(array[i]) )
            cout << array[i] << " ";

    cout << endl;
}

bool is_positive(int n) { return (n > 0); }
bool is_negative(int n) { return (n < 0); }

struct is_greater_than
{
    int val;

    is_greater_than(int v): val(v) {}

    bool operator () (int n) { return (n > val); }
};

struct is_less_than
{
    int val;

    is_less_than(int v): val(v) {}

    bool operator () (int n) { return (n < val); }
};

int main()
{
    int my_array[] = { 1, 2, -1, 3, -4, 5, 10, -9, 0, 32 };

    print_if(my_array, 10, is_positive);
    print_if(my_array, 10, is_negative);
    print_if(my_array, 10, is_less_than(2));
    print_if(my_array, 10, is_greater_than(2));

    return 0;
}

Notice that in my first two print_if calls I pass a function as the predicate,
while in the last two I pass an object of a class that overloads operator ().

In fact, I can pass anything I want as the predicate, as
long as this -> if ( pred(array[i]) ) is valid C++ code.

This means that I can't pass a member function as the predicate,
because the above is not a valid way to call a member function.

The same reasoning applies to the std::list::sort function comparator.

[Duthomhas]

Actually, there are functors in <functional> that let you pass member functions as comparitors...

[m4ster r0shi]

Yes, there are. But they are of no use here.

If I wanted to make my comparator a member function, it should
be a  member  of  std::pair< /* ... */ >,  and I can't  do that.
I guess I could make a  specialization  of std::pair to  implement
operator  <  the way I want, but that  would not be very  wise...

[EDIT]

Oops... operator < is not a member function of std::pair.
Still, writing  a  specialization  for it is not a good choice,
as you may want  to have more  than one  comparators.

[/EDIT]

Anyway,  I don't  think it's a good  idea for a  comparator  to be a  member
function,  even  if I have the  option to do it. These  functors  you  mention
are useful in  situations  where you want to use  for_each with a  member
function. Also, as you said, what you pass in that case is still a functor, so,
technically speaking, you can't pass a member function as a comparator :P