11

Can someone explain why the sort below causes seg faults? Is this a known bug with g++ (sorting vector of pointers)? I am compiling with g++ 4.5.2.

#include <iostream>
#include <algorithm>
#include <vector>

using namespace std;

typedef vector<int> A;
bool face_cmp(const A *x, const A *y) {
  return x != y;
}

int main(int argc, char* argv[]) {

  vector<A *> vec;
  for (int i=0; i<100; i++) {
    vec.push_back( new vector<int>(i%100, i*i) );
  }

  vector<A *>::iterator it;
  sort(vec.begin(), vec.end(), face_cmp);

  return EXIT_SUCCESS;
}

Compiling on codepad gives:

/usr/local/lib/gcc/i686-pc-linux-gnu/4.1.2/../../../../include/c++/4.1.2/debug/safe_iterator.h:240:
    error: attempt to decrement a dereferenceable (start-of-sequence)     
    iterator.

Objects involved in the operation:
iterator "this" @ 0x0xbf4b0844 {
type = N11__gnu_debug14_Safe_iteratorIN9__gnu_cxx17__normal_iteratorIPPN15__gnu_debug_def6vectorIiSaIiEEEN10__gnu_norm6vectorIS7_SaIS7_EEEEENS4_IS7_SB_EEEE (mutable iterator);
  state = dereferenceable (start-of-sequence);
  references sequence with type `N15__gnu_debug_def6vectorIPNS0_IiSaIiEEESaIS3_EEE' @ 0x0xbf4b0844
}

Thank you for the all the quick replies. The original comp function was: 

if (x == y) return false;
if (x->size() < y->size()) return true;
else if (x->size() > y->size()) return false;
else {
  for (register int i=0; i<x->size(); i++) {
    if ((*x)[i] < (*y)[i]) return true;
  }
  return false;
}

I just changed the first line and removed the rest. But it turns out it also suffers from not being a strict weak ordering (I forgot the case if (*x)[i] > (*y)[i]). I should probably have posted the entire function to begin with. Nevertheless, thanks again!!

Jack Cheng
  • 409
  • 3
  • 10
  • 1
    Your comparison function is bogus. It is not comparing values but just pointers - at best. – Jonathan Leffler Aug 08 '11 at 05:56
  • It's simplified to make the code shorter. It still creates a seg fault. – Jack Cheng Aug 08 '11 at 05:59
  • You are comparing pointers of vector, the comparison must be done on data – Arunmu Aug 08 '11 at 06:06
  • Then just remove the comparison function altogether. Just put ... there or something; it's distracting to see clearly broken code when it's not the actual problem. – Nicol Bolas Aug 08 '11 at 06:06
  • 4
    So, what is the actual comparison function? Because changing it to something sensible fixes the seg fault. http://ideone.com/qaaOA – Benjamin Lindley Aug 08 '11 at 06:07
  • Your comparator function is what breaks your code. (or its usage by std::sort, whatever). – Michael Foukarakis Aug 08 '11 at 06:07
  • Considering you structure of vector as a 2D array..do you want to sort each row of it ? – Arunmu Aug 08 '11 at 06:08
  • 1
    There's no problem with sorting a vector of pointers; the problem is in using not-equal instead of less-than in the comparison function. – zvrba Aug 08 '11 at 06:22
  • @ArunMu: There is no problems associated with using pointers in the comparisons (it may be meaningless in this context but it will not cause it to crash (and it is potentially useful in other contexts)). Thus the phrasing 'must be done on data' is not justified. – Martin York Aug 08 '11 at 07:32

5 Answers5

24

The comparison function must define a strict weak ordering which means that a < b and b < a cannot be both true. Your comparison function does not have this property.

It does not define any "before-after" relationship, so it's no wonder that the algorithm relying on this property does not function properly.

UncleBens
  • 40,819
  • 6
  • 57
  • 90
12

Third argument of std::sort should be a function (or functional object) such that if compare(a, b) is true then compare(b, a) should be false, but your one isn't such. So your program is UB and can give any result.

Mihran Hovsepyan
  • 10,810
  • 14
  • 61
  • 111
  • All true. But still does not really answer the question. You need to it explain the relationship that actually needs to be defined by the comparison operator. – Martin York Aug 08 '11 at 07:23
10

No your code is wrong. Comparison functions for std::sort must use < or it's equivalent, using != is not correct. Probably you want this

bool face_cmp(const A *x, const A *y) {
  return *x < *y;
}
john
  • 85,011
  • 4
  • 57
  • 81
  • The other two answers state it better, strict weak ordering is what I really meant to say :) – john Aug 08 '11 at 06:13
  • 3
    Of course you can compare vectors with < , see here http://www.cplusplus.com/reference/stl/vector/operators/ . Please remove your down votes. My answer is not as good as the others, but it's not incorrect. – john Aug 08 '11 at 06:18
  • @ArunMu Yes, you can compare vectors like that. – Benjamin Lindley Aug 08 '11 at 06:20
  • The comparison operator does **not** need to use '<' it could just as easily use '>' or a host of other comparisons. This is a weak answer because you do not define what the relationship of the comparison operator must define. – Martin York Aug 08 '11 at 07:27
  • @Martin: Yes that's the point I addressed in my first comment. However my answer does include some code so it might be useful to the OP. – john Aug 08 '11 at 09:31
3

Ensure that you're just using greater than or less than. DO NO USE equal to. Equal to will SEGFAULT with certain data sets:

// Good
bool face_cmp(const A *x, const A *y) {
  return *x < *y;
}

// Also okay for reverse sorting
bool face_cmp(const A *x, const A *y) {
  return *x > *y;
}

// This will SEGFAULT
bool face_cmp(const A *x, const A *y) {
  return *x <= *y;
}

The real danger with <= is the lack of repeatability. I had some C++ code that SEGFAULT'ed on Android, while happily running on my x86 PC. For me, the magic number was 68 elements, 67 was fine, 68 SEGFAULT'ed.

Luke Dupin
  • 2,275
  • 23
  • 30
1

Define your comparison function as

bool face_cmp(const A *x, const A *y) {
  return x < y;
}
zvrba
  • 24,186
  • 3
  • 55
  • 65