views:

45

answers:

1

Hi there. I wrote a Cocktail Sort algorithm, and am testing it by generating random vectors of size 500 up to 10,000 - running it 10 times per vector. After about the 2000-3000 length vector mark, the code segfaults. I expect it isn't the test code as the same test is used for multiple sorting algorithms and it runs fine for every other algorithm. I am assuming somewhere I miss an end-condition and it tries to access an element of the input array that doesn't exist... but I'm not entirely sure if that would cause a segfault to be run.

This is the code, I hope someone can spot my error. (I also would enjoy any comments on how it could be better - but please note I do value readability over speed for this code.)

void Sorting::cocktailSort(vector<int>& A) {

    int temp;

    // The first/last indexes to check. Anything before/after these indexes
    // is already sorted.
    int firstIndex = -1;
    int lastIndex = A.size()-1;
    bool swapped;

    do {
        firstIndex += 1;
        swapped = false;
        for(int i = firstIndex-1; i < lastIndex; i++) {
            if(A[i] > A[i+1]) {
                temp = A[i];
                A[i] = A[i+1];
                A[i+1] = temp;
                swapped = true;
            }
        }
        if(!swapped) break;
        swapped = false;
        lastIndex -= 1;
        for(int i = lastIndex; i >= firstIndex; i--) {
            if(A[i] < A[i-1]) {
                temp = A[i];
                A[i] = A[i-1];
                A[i-1] = temp;
                swapped = true;
            }
        }
    }while (swapped);

}

This is not homework.

+3  A: 

If you use A.at(i) instead of A[i], bounds checking will be done, and out-of-range exceptions thrown. That may be helpful for debugging.

It appears to me that the access here...

    for(int i = firstIndex-1; i < lastIndex; i++) {
        if(A[i] > A[i+1]) {

will be out-of-bounds when firstIndex is zero (the first iteration of the main loop).

Brian Hooper
... Indeed it is. I have no idea why I set firstIndex to -1 instead of 0 when initialising it. Running the test again now, but this looks very likely to be the culprit. You have no idea how many times I looked over this and missed this problem XD.
Stephen
Just finished running the test code through twice with nary an error to be seen. Brian, thank you very much. An upvote for the `at()` idea (which would have pointed this straight out to me, since it `out_of_range` s on the access attempt), and a green tick for finding the fault too. As a question - I assume it took so long to seg-fault because for most of the time the memory it accessed doing A[-1] was actually valid memory? And then eventually it accessed and tried to change an un-allowed bit of memory?
Stephen
@Stephen Possibly. Either that or writing to `A[i]` overwrote a part of something else in your program that later caused that other thing to access an un-allowed memory address.
Tyler McHenry
More generally: it's best always to use `at` whenever there is no performance issue... and only to worry about performance issue when the profiler say so.
Matthieu M.
@Matthieu M.: But `at` introduces quite a performance issue, doesn't it? When using it to debug my cocktail sort here I found that the sort slowed *way* down. Now, this could have been incidental, perhaps the processor was just more taxed than normal and it co-incided, but it seems to me that direct access is alot faster... even if it does mean you have a problem if you screw up like I did XD.
Stephen
`at` doesn't normally introduce much of a performance issue. As always with performance one needs to measure. It IS possible that it would slow down the machine though, but generally in debug you don't care much so you can always use `#ifdef` / `#else` / `#endif` to use `at` in debug and `[]` in release.
Matthieu M.