0

I'm writing code for a project in my class, and it is producing incredibly unusual results. Row and column values end up with random numbers in the millions and it eventually throws exceptions and I can't understand why.

#include <iostream>
using namespace std;

class SparseRow {
protected:
    int row;
    int col;
    int value;
public:
    SparseRow();
    SparseRow(int inRow, int inCol, int inVal);
    SparseRow(const SparseRow& newRow);
    SparseRow& operator=(const SparseRow& newRow);
    ~SparseRow();
    void display();
    int getRow();
    int getCol();
    int getValue();
    void setRow(int inRow);
    void setCol(int inCol);
    void setValue(int inVal);
};

class SparseMatrix {
protected:
    int noRows;
    int noCols;
    int commonValue;
    int noSparseValues;
    SparseRow *myMatrix;
    void setNoSparseValues(int noSV);
public:
    SparseMatrix();
    SparseMatrix(int rows, int cols, int cv, int noSV);
    SparseMatrix(const SparseMatrix& matrix);
    SparseMatrix& operator=(const SparseMatrix& matrix);
    ~SparseMatrix();
    void readMatrix();
    SparseMatrix* transpose();
    SparseMatrix* multiply(SparseMatrix& M);
    SparseMatrix* add(SparseMatrix& M);
    void display();
    void displayMatrix();
    int getNoRows();
    int getNoCols();
    int getCommonValue();
    int getNoSparseValues();
    int getValue(int row, int col);
    void setValue(int row, int col, int val);
};

SparseRow::SparseRow() {
    row = -1;
    col = -1;
    value = 0;
}

SparseRow::SparseRow(int inRow, int inCol, int inVal) {
    row = inRow;
    col = inCol;
    value = inVal;
}

SparseRow::SparseRow(const SparseRow& newRow) {
    row = newRow.row;
    col = newRow.col;
    value = newRow.value;
}

SparseRow& SparseRow::operator=(const SparseRow& newRow) {
    if (this != &newRow) {
        row = newRow.row;
        col = newRow.col;
        value = newRow.value;
    }
    return *this;
}

SparseRow::~SparseRow() {
    cout << "Deleting SparseRow with values: " << endl;
    display();
}

void SparseRow::display() {
    cout << "Row: " << row << ", Column: " << col << ", Value: " << value << endl;

}

int SparseRow::getRow() {
    return row;
}

int SparseRow::getCol() {
    return col;
}

int SparseRow::getValue() {
    return value;
}

void SparseRow::setRow(int inRow) {
    row = inRow;
}

void SparseRow::setCol(int inCol) {
    col = inCol;
}

void SparseRow::setValue(int inVal) {
    value = inVal;
}

SparseMatrix::SparseMatrix() {
    noRows = 0;
    noCols = 0;
    commonValue = 0;
    noSparseValues = 0;
}

SparseMatrix::SparseMatrix(int rows, int cols, int cv, int noSV) {
    noRows = rows;
    noCols = cols;
    commonValue = cv;
    noSparseValues = noSV;
    myMatrix = new SparseRow[rows * cols];
}

SparseMatrix::SparseMatrix(const SparseMatrix& matrix) {
    noRows = matrix.noRows;
    noCols = matrix.noCols;
    commonValue = matrix.commonValue;
    noSparseValues = matrix.noSparseValues;
    myMatrix = matrix.myMatrix;
}

SparseMatrix& SparseMatrix::operator=(const SparseMatrix& matrix) {
    if (this != &matrix) {
        delete[] myMatrix;

        noRows = matrix.noRows;
        noCols = matrix.noCols;
        commonValue = matrix.commonValue;
        noSparseValues = matrix.noSparseValues;
        myMatrix = matrix.myMatrix;
    }
    return *this;
}

SparseMatrix::~SparseMatrix() {
    cout << "Deleting SparseMatrix with values: " << endl;
    display();

    delete[] myMatrix;
}

void SparseMatrix::readMatrix() {
    int count = 0;
    int val;

    for (int i = 0; i < noRows; i++) {
        for (int j = 0; j < noCols; j++) {
            cin >> val;
            if (val != commonValue) {
                myMatrix[count].setRow(i);
                myMatrix[count].setCol(j);
                myMatrix[count].setValue(val);
                // Why does C++ prevent me from calling a constructor on objects in an array? It doesn't make sense.
                count++;
            }
        }
    }

    if (count != noSparseValues) {
        cout << "ERROR: Incorrect number of sparse values! Changing to correct number." << endl;
        noSparseValues = count;
    }
}

SparseMatrix* SparseMatrix::transpose() {
    SparseMatrix *newMatrix = new SparseMatrix(noCols, noRows, commonValue, noSparseValues);

    for (int i = 0; i < noSparseValues; i++) {
        newMatrix->setValue(myMatrix[i].getCol(), myMatrix[i].getRow(), myMatrix[i].getValue());
    }

    return newMatrix;
}

SparseMatrix* SparseMatrix::multiply(SparseMatrix& M) {
    if (noCols != M.getNoRows()) {
        cout << "ERROR: Matrices cannot be multiplied!" << endl;
        return NULL;
    }

    SparseMatrix *newMatrix = new SparseMatrix(noRows, M.getNoCols(), commonValue, noRows * M.getNoCols());
    // Why does C++ prevent me from creating an array to store this information? It doesn't make sense.

    int SVCount = 0;
    for (int i = 0; i < noRows; i++) {
        for (int j = 0; j < M.getNoCols(); j++) {
            int sum = 0;
            for (int k = 0; k < noCols; k++) {
                sum += getValue(i, k) * M.getValue(k, j);
            }
            if (sum != newMatrix->getCommonValue()) {
                SVCount++;
            }
            newMatrix->setValue(i, j, sum);
        }
    }
    newMatrix->setNoSparseValues(SVCount);

    return newMatrix;
}

SparseMatrix* SparseMatrix::add(SparseMatrix& M) {
    if (noRows != M.getNoRows() || noCols != M.getNoCols()) {
        cout << "ERROR: Matrices cannot be added!" << endl;
        return NULL;
    }

    SparseMatrix *newMatrix = new SparseMatrix(noRows, noCols, commonValue + M.getCommonValue(), noRows * noCols);

    int SVCount = 0;
    for (int i = 0; i < noRows; i++) {
        for (int j = 0; j < noCols; j++) {
            int sum = getValue(i, j) + M.getValue(i, j);
            if (sum != newMatrix->getCommonValue()) {
                SVCount++;
            }
            newMatrix->setValue(i, j, sum);
        }
    }

    newMatrix->setNoSparseValues(SVCount);

    return newMatrix;
}

void SparseMatrix::display() {
    for (int i = 0; i < noSparseValues; i++) {
        myMatrix[i].display();
    }
}

void SparseMatrix::displayMatrix() {
    for (int i = 0; i < noRows; i++) {
        for (int j = 0; j < noCols; j++) {
            cout << getValue(i, j) << " ";
        }
        cout << endl;
    }
}

int SparseMatrix::getNoRows() {
    return noRows;
}

int SparseMatrix::getNoCols() {
    return noCols;
}

int SparseMatrix::getCommonValue() {
    return commonValue;
}

int SparseMatrix::getNoSparseValues() {
    return noSparseValues;
}

void SparseMatrix::setNoSparseValues(int noSV) {
    noSparseValues = noSV;
}

int SparseMatrix::getValue(int row, int col) {
    for (int i = 0; i < noSparseValues; i++) {
        if (myMatrix[i].getRow() == row && myMatrix[i].getCol() == col) {
            return myMatrix[i].getValue();
        }
    }
    return commonValue;
}

void SparseMatrix::setValue(int row, int col, int val) {
    bool replacingSparse = (getValue(row, col) != commonValue);
    bool replacingWithSparse = (val != commonValue);

    int index = -1;

    if (replacingSparse) {
        for (int i = 0; i < noSparseValues; i++) {
            if (myMatrix[i].getRow() == row && myMatrix[i].getCol() == col) {
                index = i;
                break;
            }
        }
        if (replacingWithSparse) {
            myMatrix[index].setValue(val);
        }
        else {
            for (int i = index; i < noSparseValues; i++) {
                myMatrix[i] = myMatrix[i + 1];
            }
            noSparseValues--;
        }
    }
    else {
        if (replacingWithSparse) {
            for (int i = 0; i < noSparseValues; i++) {
                if (myMatrix[i].getRow() > row || (myMatrix[i].getRow() >= row && myMatrix[i].getCol() > col)) {
                    index = i;
                    break;
                }
            }
            for (int i = noSparseValues; i > index; i--) {
                myMatrix[i] = myMatrix[i - 1];
            }
            myMatrix[index].setRow(row);
            myMatrix[index].setCol(col);
            myMatrix[index].setValue(val);
            noSparseValues++;
        }
    }
}

int main() {
    int n, m, cv, noNSV;
    SparseMatrix* temp;

    cin >> n >> m >> cv >> noNSV;
    SparseMatrix* firstOne = new SparseMatrix(n, m, cv, noNSV);
    firstOne->readMatrix();

    cin >> n >> m >> cv >> noNSV;
    SparseMatrix* secondOne = new SparseMatrix(n, m, cv, noNSV);
    secondOne->readMatrix();

    cout << "First one in sparse matrix format" << endl;
    firstOne->display();

    cout << "First one in normal matrix format" << endl;
    firstOne->displayMatrix();

    cout << "Second one in sparse matrix format" << endl;
    secondOne->display();

    cout << "Second one in normal matrix format" << endl;
    secondOne->displayMatrix();

    cout << "After Transpose first one in normal format" << endl;
    temp = firstOne->transpose();
    temp->displayMatrix();

    cout << "After Transpose second one in normal format" << endl;
    temp = secondOne->transpose();
    temp->displayMatrix();

    cout << "Multiplication of matrices in sparse matrix form:" << endl;
    temp = secondOne->multiply(*firstOne);
    temp->display();

    cout << "Addition of matrices in sparse matrix form:" << endl;
    temp = secondOne->add(*firstOne);
    temp->display();
}

Input:

3 3 0 3
2 2 2
0 0 0
0 0 0

3 3 0 3
2 2 2
0 0 0
0 0 0

Expected output: (not formatted exactly the same, but the numbers should be identical)

First one in sparse matrix format
0, 0, 2
0, 1, 2
0, 2, 2
First one in normal matrix format
2     2     2
0     0     0
0     0     0
Second one in sparse matrix format
0, 0, 2
0, 1, 2
0, 2, 2
Second one in normal matrix format
2     2     2
0     0     0
0     0     0
After Transpose first one
2     0     0
2     0     0
2     0     0
After Transpose second one
2     0     0
2     0     0
2     0     0
Multiplication of matrices in sparse matrix form:
0, 0, 4
0, 1, 4
0, 2, 4
Addition of matrices in sparse matrix form:
0, 0, 4
0, 1, 4
0, 2, 4

Actual output:

First one in sparse matrix format
Row: 0, Column: 0, Value: 2
Row: 0, Column: 1, Value: 2
Row: 0, Column: 2, Value: 2
First one in normal matrix format
2 2 2
0 0 0
0 0 0
Second one in sparse matrix format
Row: 0, Column: 0, Value: 2
Row: 0, Column: 1, Value: 2
Row: 0, Column: 2, Value: 2
Second one in normal matrix format
2 2 2
0 0 0
0 0 0
After Transpose first one in normal format
0 0 0
2 0 0
2 0 0
After Transpose second one in normal format
0 0 0
2 0 0
2 0 0
Multiplication of matrices in sparse matrix form:
Row: 0, Column: 1, Value: 4
Row: 0, Column: 2, Value: 4
Row: 369, Column: -33686019, Value: 9
Addition of matrices in sparse matrix form:
(Exceptions thrown here, line 222 in code)

Exceptions thrown:

Project1.exe has triggered a breakpoint.

Unhandled exception at 0x777E8499 (ntdll.dll) in Project1.exe: 0xC0000374: A heap has been corrupted (parameters: 0x77825890).
draket333
  • 9
  • 2
  • 3
    I would suggest that modern developers almost *never* have a reason to use naked `new` and `delete`, when smart pointers are so much better. – paxdiablo Feb 07 '19 at 05:29
  • I forgot to mention, I'm used to Java and very new to C++, so I really don't know how most of these things work. For example, I've heard of a "double pointer," but I have no idea what it is or how to use it. – draket333 Feb 07 '19 at 05:50
  • 4
    If you are new to C++, I highly recommend reading [a good book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list), rather than trying to figure it out based on your knowledge of Java. Smart pointers (like `std::unique_ptr`) are great; they automatically manage the lifetime of the pointer, freeing you from having to call `delete` yourself. In this case, you don't even seem to need smart pointers. You could get away with `std::vector`. – Cody Gray Feb 07 '19 at 06:17
  • The problem first arises in the first transpose, and since we know `SparseMatrix::display()` works from the input matrices, `SparseMatrix::transpose()` must be wrong. Try putting a `std::cerr` line inside the loop to see how many values are actually being set (should be 3 for the first transpose, may only be 2). If only 2 values are being set, you have to figure out why the third one doesn't happen (probably the count is wrong); if all 3 are "set", you have to figure out why nothing is recorded inside the matrix (maybe it's written to an invalid address). – Baryons for Breakfast Feb 07 '19 at 07:01
  • 1
    `for (int i = 0; i < noSparseValues; i--)` What do you think is going on here? – n. 'pronouns' m. Feb 07 '19 at 08:26
  • @CodyGray Using vectors sounds like a wonderful idea, but unfortunately my professor specifically requires no #include statements on this project other than iostream, and if I understand it correctly, you have to use #include to use vectors. – draket333 Feb 07 '19 at 15:18
  • @n.m. I'm really not sure how I missed that. I had a lot of issues related to the next for loop after that because it had i++ instead of i--. Thanks for pointing it out! – draket333 Feb 07 '19 at 15:19
  • @BaryonsforBreakfast I'm not certain, but I think this is the root of the issue: Row: 369, Column: -33686019, Value: 9 I would assume the same thing happens in the transpose method, although I could test it and make sure. But why does that happen? – draket333 Feb 07 '19 at 15:23
  • 1
    Is the use of the auxiliary class SparseRow a design choice of yours or is it mandatory for the assignment? I find its name misleading at best, it doesn't model a row, not to mention a sparse one and it could be replaced for any means by a `struct Element {int row, col, value;};`, if needed at all. You may want to look at other possible implementations of [sparse matrices](https://en.wikipedia.org/wiki/Sparse_matrix), if you are allowed to. – Bob__ Feb 07 '19 at 15:44
  • 1
    @Bob__ I definitely agree with you (except that I don't know what `struct Element` means), but SparseRow is a part of the assignment and I can't do much about it. I can't even rename it to something more accurate. – draket333 Feb 07 '19 at 15:58
  • Turn on more warnings (MSVC: warning level 4, gcc: -Wall -Wextra) and check the compiler output. Learn how to use a debugger. The code that you post is much too many lines long to easily be checked by us. – Werner Henze Feb 07 '19 at 16:46
  • 1
    @draket333 Well, it's another keyword used to define a class, the only difference is that the member of a class defined with the keyword `class` are private by default, while the members of a class defined with the keyword `struct` are public by default. My point is that for every member of `SparseRow` you define getters and setters that expose them without any check, so you could just make those members public and access them directly. *Unless* the interface of your objects is defined by your assignment. – Bob__ Feb 07 '19 at 17:08

1 Answers1

1

I'll address only some of the issues in the posted code.

pointers

Their explict use is always prone to error and it's increasingly discouraged at every new version of the standard. In OP's code they own the responsability of managing memory resources, but in C++ they don't automatically call the destructor of an object when it goes out of scope. That's what a smart pointer like std::unique_ptr can do.

Let's see what happens in main:

int n, m, cv, noNSV;
SparseMatrix* temp;  // <- Declaration far from initialization

cin >> n >> m >> cv >> noNSV;
SparseMatrix* firstOne = new SparseMatrix(n, m, cv, noNSV);
//          ^^^^^^^^^^^^^^^^ Why?    
// ...
temp = firstOne->transpose();  // <- Here the pointer is initialized
// ...
temp = secondOne->transpose(); // <- Here the pointer is overwritten, but the allocated memory
                               //    isn't released, it leaks.
// ...
temp = secondOne->multiply(*firstOne); // <- Again...
// ...
temp = secondOne->add(*firstOne);  // <- ...and again
// ...
// No 'delete' calls at the end, so no destructor is called for the previously allocated objects 

The only reason to use pointers here, is that SparseMatrix::transpose() is designed to return one, but it could easily retrun a SparseMatrix instead (Return Value Optimization would avoid unnecessary copies).

To every call of new should correspond a delete and since functions like transpose return a pointer to a newly allocated objects, delete temp; should be called before overwriting temp, to make sure that the proper destructor is called.

In C++, though, we should take advantage of RAII (Resource Acquisition Is Initialization, also named Scope-Bound Resource Management):

{   // <- Start of a scope
    int n, m, cv, noNSV;

    std::cin >> n >> m >> cv >> noNSV;
    // Declare a variable using the constructor. Its lifetime begins here.     
    SparseMatrix firstOne(n, m, cv, noNSV);
    // The same for 'secondOne'
    // ...
    // SparseSparse::transpose() here should return a SparseMatrix, not a pointer
    SparseMatrix temp = firstOne.transpose(); 

    // ...
    // Here the matrix is reassigned (which can be cheap if move semantic is implemented) 
    temp = secondOne.transpose();
    // ...
    temp = secondOne.multiply(firstOne); // <- Again...
    // ...
 }  // <- End of scope, all the destructors are called. No leaks. 

special member functions

That's actually related to previous point, looking at how e.g. the copy assignment operator is implemented in the posted snippet:

 SparseMatrix& SparseMatrix::operator=(const SparseMatrix& matrix) {
     if (this != &matrix) {     // <- debatable, see copy-and-swap idiom
         delete[] myMatrix;     // <- putted here makes this function not exception safe ^^

         noRows = matrix.noRows;
         noCols = matrix.noCols;
         commonValue = matrix.commonValue;
         noSparseValues = matrix.noSparseValues;
         myMatrix = matrix.myMatrix; // <- The pointer is overwritten, but this is shallow copy
                                     //    what you need is a deep copy of the array
     }
     return *this;
 }

A proper implementation of this and the other special functions would take advantage of the copy-and-swap idiom, which is masterfully described here:

What is the copy-and-swap idiom?

Also, regarding this comment in OP's reading function:

for (int i = 0; i < noRows; i++) {
    for (int j = 0; j < noCols; j++) {
        cin >> val;
        if (val != commonValue) {
            myMatrix[count].setRow(i);
            myMatrix[count].setCol(j);
            myMatrix[count].setValue(val);
            // Why does C++ prevent me from calling a constructor on objects in an array? It doesn't make sense.
            count++;
        }
    }
}

The asker could use the placement parameter of new (placement new), there:

for (int i = 0; i < noRows; i++) {
    for (int j = 0; j < noCols; j++) {
        cin >> val;
        if (val != commonValue) {

            new (myMatrix + count) SparseRow(i, j, val);

            count++;
        }
    }
}

Many other issues are left to the OP to solve.

Bob__
  • 9,461
  • 3
  • 22
  • 31