Thread: const parameter of function is being changed

I don't know why it is doing this. I passed it as const and in the function I don't make any changes to the viable.

The "intersectionOfSets" function changes the class instance "empty"'s array "set"'s first position from 0 to 5383480.

Please correct me if my vocab is off, thanks for the help!


main.cpp

Code:

#include <iostream>
#include "IntegerSet.h"

using namespace std;


int main()
{
    const int arraySize = 10;
    int array[10] = {0,1,1,0,1,0,0,1,1,0};

    IntegerSet five (10);
    IntegerSet empty (10);

    IntegerSet Union(10);
    IntegerSet intersection(10);
    IntegerSet arraySet (array, arraySize);

    cout << "Empty Set:\n";
    empty.printSet();

    five.insertElement(5);
    cout << "\n\nEmpty set with 5 added to set:\n";
    five.printSet();

    cout << "\n\nUnion of empty set and set with 5 in it:\n";
    five.unionOfSets(empty,Union);
    Union.printSet();

    cout << "\n\n***Empty before:\n";
    empty.printSet();


    cout << "\n\nIntersection of empty set and set with 5 in it\n";
    five.intersectionOfSets(empty, intersection); // problem ith empty changing here
    intersection.printSet();

    cout << "\n\n***Empty after:\n";
    empty.printSet();





    cout << "\n\nChecks if the set with 5 in it is equal to the empty set: (0 = false, 1 = true)\n";
    cout << five.isEqual(empty);

    five.deleteElement(5);
    cout << "\n\nDeleted element 5:\n";
    five.printSet();




    cout << "\n\nChecks if the previous set with 5 with 5 just being taken away (empty) is equal to the empty set:(0 = false, 1 = true)\n";
    cout << five.isEqual(empty);

    cout << "\n\nOutput integer set that is predefined with an array {0,1,1,0,1,0,0,1,1,0}:\n";
    arraySet.printSet();


    return 0;
}

IntegerSet.cpp

Code:

#include <iostream>
#include "IntegerSet.h"

IntegerSet::IntegerSet(int tempSize)
{
    size = tempSize;
    set = new int[size];

    for(int a = 0; a < size; a++)
        set[a] = 0;
}

IntegerSet::IntegerSet(int arraySet[], int tempArraySize)
{
    size = tempArraySize;
    set = new int[size];

    for(int a = 0; a < size; a++)
        set[a] = arraySet[a];
}

IntegerSet::~IntegerSet()
{
    delete [] set;
}



void IntegerSet::unionOfSets(const IntegerSet tempB, IntegerSet &newSet)
{
    if(size == tempB.size)
    {
        for(int a = 0; a < size; a++)
        {
            if(set[a] == 1 || tempB.set[a] == 1)
                newSet.set[a] = 1;

            else
                newSet.set[a] = 0;
        }
    }

    else
        std::cout << "Error. Sets being compared are not the same length.";

}

void IntegerSet::intersectionOfSets(const IntegerSet tempB, IntegerSet &newSet)
{
    if(size == tempB.size)
    {
        for(int a = 0; a < size; a++)
        {
            if(set[a] == 1 && tempB.set[a] == 1)
                newSet.set[a] = 1;

            else
                newSet.set[a] = 0;
        }
    }


    else
        std::cout << "Error. Sets being compared are not the same length.";
}

void IntegerSet::insertElement(int element)
{
    if(element < size &&  element > -1 )
        set[element] = 1;

    else
        std::cout << "Error. Inserted element is not in scope.\n";
}

void IntegerSet::deleteElement(int element)
{
    if( element < size &&  element > -1 )
        set[element] = 0;

    else
        std::cout << "Error. Inserted element is not in scope.\n";
}

void IntegerSet::printSet() const
{
    for(int a = 0; a < size; a++)
    {
        if(set[a] == 1)
            std::cout << a << " ";

        else
            std::cout << set[a] << " ";
    }
}

bool IntegerSet::isEqual(const IntegerSet tempB) const
{
    for(int a = 0; a < size; a++)
    {
        if(set[a] != tempB.set[a])
            return false;
    }

    return true;
}

IntegerSet.h

Code:

class IntegerSet
{
    private:
    int *set;
    int size;

    public:
    IntegerSet(int);
    IntegerSet(int[], int);
    ~IntegerSet();


    void unionOfSets(const IntegerSet tempB, IntegerSet &newSet);
    void intersectionOfSets(const IntegerSet tempB, IntegerSet &newSet);

    void insertElement(int);
    void deleteElement(int);

    void printSet() const;

    bool isEqual(const IntegerSet tempB) const;

};

One problem I see is that you defined the destructor, but you did not also define (or disable) the copy constructor and copy assignment operator. This is compounded by those parameters where you use pass by value instead of pass by const reference: what happens is that a copy of your object is made, but this copy has a copy of the set pointer that points to the same elements. Thus, when the copy goes out of scope, the destructor is invoked to destroy these elements, even though they still belong to the original object.

Hence, what you should do is to implement the copy constructor and copy assignment operator. It would also be a good idea to have const reference parameters when you don't actually need a copy.

Undefined behavior, my friend. You should pass your const objects as references (otherwise there's not much point in making them const). Do that for unionOfSets, intersectionOfSets, and isEqual. And of course laserlight is giving some important advice.


A couple of other points:


It seems odd to me that you use 'a' as your index variable since most people use i. But maybe that's just me.


Your use of the word temp for many parameter names is wrong. They aren't really "temp" values, they're function parameters. If you wish to label them you might use "param", like paramB, or "in" like set_in. In general, using the word temp is a last resort since it's not very descriptive.


Stuff like this

Code:

            if(set[a] == 1 || tempB.set[a] == 1)
                newSet.set[a] = 1;
            else
                newSet.set[a] = 0;

can be written like this

Code:

            newSet.set[a] = set[a] || tempB.set[a];

You could send error messages to cerr instead of cout. This allows the user to redirect error messages separately from regular output.
Actually, it is unusual for low-level routines to print error messages. Typically they indicate that an error occured (returning a flag or throwing an exception) and let the caller decide what to do.


You can split long strings like this

Code:

    cout << "\n\nChecks if the previous set with 5 with 5 just being taken away "
            "(empty) is equal to the empty set:(0 = false, 1 = true)\n";

Maybe I'm old-fashioned, but it seems ridiculous to me to have a line 140 chars long. Pick some max size (80 is common) and don't go beyond that.

Originally Posted by oogabooga

Stuff like this

Code:

if(set[a] == 1 || tempB.set[a] == 1)
    newSet.set[a] = 1;
else
    newSet.set[a] = 0;

can be written like this

Code:

newSet.set[a] = set[a] || tempB.set[a];

I don't like making use of the conversion from bool to int: it usually is semantically incorrect. Looking at the code here, I think that it is acceptable, but that is because the set member variable should be a pointer to bool, not int, since the integers are represented by the indices of the set dynamic array (which is quite limiting, methinks) such that the corresponding elements are true or false.

Originally Posted by laserlight

I don't like making use of the conversion from bool to int: it usually is semantically incorrect. Looking at the code here, I think that it is acceptable, but that is because the set member variable should be a pointer to bool, not int, since the integers are represented by the indices of the set dynamic array (which is quite limiting, methinks) such that the corresponding elements are true or false.

Good point. I was probably thinking in C (as usual) so not even considering a conversion.
I agree that the element members should be bool, not int.

Ah ok, thank you Sorry for late response. Problems dealt with and program is working as well as optimal (good point on the const).

Thanks again