3

here is the problem:

In my first class i have a vector, a double variable and I overload the comparison operators. Here is the relevant code:

class City
{
    double distance;
    std::vector<int> coordinates;

    bool operator<(const City& city) const
    {   
        return this->distance < city.distance;
    } 

    // same for the greater-than operator but changing "<" to ">"
};

In another class I have a vector of cities, which I have to sort every time a condition is met. For that I have a struct defined as follows:

EDIT: (reference instead of value)

struct CitySortHelper {
    bool operator() (const City &x, const City &y) const { return x < y; } 
} city_sort;

Now the problem part, when I sort the vector new City objects appear, and I can't explain why:

EDIT:

// this prints all current objects in the vector
for (int i = 0; i < totalCities; i++) {
    std::cout << cities->at(i) << std::endl;
}

// after the following line I get new City objects in the 
// vector, that weren't there before the sort. The new objects
// always have distance = 0 and random values in the coordinates
std::sort(cities->begin(), cities->end(), city_sort);

// using the sort with no predicate also gives the same faulty results
std::sort(cities->begin(), cities->end());

EDIT: (the copy constructor and assignment operator)

City(const City &city)
{
    this->distance = city.distance;
    this->coordinates = city.coordinates;
}

City& operator= (const City &city)
{
    this->distance = city.distance;
    this->coordinates = city.coordinates;

    return *this;
}

The weird part is that this only happens if I sort the City objects in ascending order, i.e. if I change the comparator operator in the CitySortHelper from "<" to ">" everything works fine.

Any ideas why this happens ?? Any help is appreciated.

6
  • Do you have a specialized copy constructor for your City class? Dec 19, 2011 at 1:51
  • Why do you need a custom predicate at all if you can just use std::sort(cities->begin(), cities->end()) and std::sort(cities->begin(), cities->end(), std::greater<City>)?
    – Kerrek SB
    Dec 19, 2011 at 1:57
  • @KerrekSB I also tried without the predicate. In the normal case i.e. std::sort(cities->begin(), cities->end()) where the less-operator is used, I still get those new objects. Whereas using std::greaterno problems.
    – iska
    Dec 19, 2011 at 2:06
  • I can't reproduce your issue. You'll need to provide a complete, compilable example that demonstrates the problem. Dec 19, 2011 at 2:48
  • you say that you can use std::greater, but I don't see any City::operator>(). On all of the STL impls that I've looked at (ie gcc and that's it) std::greater() just forwards to operator>. If you have defined City::operator>, then it might be helpful to post that.
    – gred
    Dec 19, 2011 at 21:26

3 Answers 3

4

CitySortHelper needs to take parameters by const reference, not by value. Another thing to keep in mind is that sort uses assignment operator for the City; check that your assignment operator is working correctly. Taking care of these two issues should fix the problem.

8
  • I've changed the CitySortHelper and checked the assignment operator and the copy constructor. The problem persists. It works for descending but fails for ascending order.
    – iska
    Dec 19, 2011 at 2:02
  • @iska Could you please update the question to reflect the changes that you have made? Dec 19, 2011 at 2:07
  • Here you go. Right now I am sticking with ordering in descending order then reversing the vector :(
    – iska
    Dec 19, 2011 at 2:21
  • @iska Are you certain that totalCities equals cities->size()? I just copied your code into a CPP file, and it sorted OK in ascending order. Dec 19, 2011 at 2:34
  • Yes, in fact it was among the first things I checked before asking here.
    – iska
    Dec 19, 2011 at 2:40
1

Change your sort helper to have

bool operator() ( const City& x , const City& y) const

And also check that City copy constructor and assignment operator do the proper thing

1
  • I've changed the CitySortHelper and checked the assignment operator and the copy constructor. The problem persists. It works for descending but fails for ascending order.
    – iska
    Dec 19, 2011 at 2:03
1

You shouldn't use std::sort() if you want to preserve order, you should use std::stable_sort(). stable_sort guarantees elements maintain their relative order, sort doesn't.

Also, it doesn't seem like sort is your problem here. It seems there are City objects getting pushed into the vector somewhere, and you aren't noticing them because you're checking on a variable for size instead of the vector's iterators. Try printing like this instead and tell us what comes out:

for (std::vector <City> ::iterator it = cities->begin(); it != cities->end(); ++it) {
    std::cout << *it << std::endl;
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.