views:

99

answers:

3

Hi, I wrote this method to find the minor of a sparse matrix:

SpMatrixVec SparseMatrix::minor(SpMatrixVec matrix, int col) const{

    SpMatrixVec::iterator it = matrix.begin();

    int currRow = it->getRow();

    int currCol = col;

    while(it != matrix.end()) {

        if(it->getRow() == currRow || it->getCol() == currCol){
            matrix.erase(it);
        }
        // if we have deleted an element in the array, it doesn't advance the
        // iterator and size() will be decreased by one.
        else{   
            it++;
        }

    }

    // now, we alter the cells of the minor matrix to be of proper coordinates.
    // this is necessary for sign computation (+/-) in the determinant recursive
    // formula of detHelper(), since the minor matrix non-zero elements are now
    // in different coordinates. The row is always decreased by one, since we
    // work witht he first line, and the col is decreased by one if the element
    // was located after 'col' (which this function receives as a parameter).

    //change the cells of the minor to their proper coordinates.
    for(it = matrix.begin(); it != matrix.end(); it++){

        it->setRow(it->getRow()-1);

        int newY;
        newY = (it->getCol() > col) ? it->getCol() + 1 : it->getCol();

        it->setCol(newY);
    }
    return matrix;

}

Now, i'm probably doing something wrong, because when reaching the second interation of the while loop, the program crashes. The basic idea was to go over the vector, and see if it is the relevant coordinate, and if so - to delete it. I increment the iterator only if there was no deletion (and in this case, the vector should update the iterator to be pointing the next element..unless i got these things wrong).

Where is the problem?

Thanks a lot.

A: 

You can't change a collection while you're iterating between its elements. Use another temp collection to store the partial results.

edit: Even better, use a functor to delete elements: remove_if You write the condition.

vulkanino
That's just plain wrong. You may modify it while iterating, you just have to ensure that the iterators aren't invalidated while you do it.
ereOn
No, it is possible to do it. You just have to store the return value of `erase` to get a new, valid iterator.
Benoit
+4  A: 

erase invalidates your iterator. Do it = matrix.erase(it) instead.

Benoit
+5  A: 

erase() invalidates your iterator.

You must update it using the return value of erase() for the loop to work:

while(it != matrix.end()) {

    if(it->getRow() == currRow || it->getCol() == currCol){
        //matrix.erase(it);
        it = matrix.erase(it); // Here is the change
    }
    // if we have deleted an element in the array, it doesn't advance the
    // iterator and size() will be decreased by one.
    else{   
        //it++;
        ++it; // ++i is usually faster than i++. It's a good habit to use it.
    }

}
ereOn
+1 but `++it` is better
Steve Townsend
@Steve Townsend: Absolutely, I wasn't checking for any other "error" in the code but you're indeed absolutely right. Fixed. Thanks.
ereOn