c++ programming help

[mansoor_]

Can someone explain to me why this piece of code fails:

 

I get a segmentation fault after writeStoredData() writes 0,1,2,3,4 to the console.

#include <iostream>

//Class storing data
class DataStorage
{
public:
    DataStorage()
    {
    }

    ~DataStorage()
    {
        delete x;
    }

    void storeThis(int* xIn, unsigned sizeIn)
    {
        x = xIn;
        size = sizeIn;
    }

    void writeStoredData()
    {
        for (unsigned i=0; i<size; ++i) {
            std::cout << x[i] << std::endl;
        }
    } 

    bool exist(int value)
    {
        for (unsigned i=0; i<size; ++i) {
            if (x = value) {
                return true;
            }
        }
        return true;
    } 

private:
    int *x;
    unsigned size;
}; 

//Stores and writes the stored data
void exercise1()
{
    std::cout << "Exercise 1" << std::endl;
    int x[5] = {0,1,2,3,4};
    DataStorage dataStorage;
    dataStorage.storeThis(x,5);
    dataStorage.writeStoredData();
} 

int main(int argc, char* argv[])
{
    exercise1();
    return 0;
}
 

what simple changes could I make to solve this problem?

[Pinguinsan]

7 minutes ago, mansoor_ said:

Can someone explain to me why this piece of code fails:

 

 

what simple changes could I make to solve this problem?

What isn't working?

[mansoor_]

1 minute ago, Pinguinsan said:

What isn't working?

I get a segmentation fault after writeStoredData() writes 0,1,2,3,4 to the console.

[BaldPanda]

First, your exist function wont work for 2 reasons (I let you find them ;))

 

For your issue :

        for (unsigned i=0; i<size; ++i) {
            std::cout << x << std::endl;
        }

X never change => I don't see how it is printing 0 1 2 3 4.

More than that, you print x (the pointer) not *x (the pointed value).

[Mira Yurizaki]

Leave the destructor empty.

 

The reason is the array int x[5] = {0,1,2,3,4}; is only alive within the scope of exercise1. Once the function returns, it calls dataStorage's destructor, but since that array no longer exists and it's trying to delete it, the destructor is trying to do things to a memory location it doesn't own (and it probably never owned it anyway). You only need to delete things if the object created any new data in the heap (i.e., invoking the new keyword).

 

There's also some other errors but I won't point them out. That's an exercise for you to fix.

[Unimportant]

2 hours ago, mansoor_ said:

Can someone explain to me why this piece of code fails:

 

I get a segmentation fault after writeStoredData() writes 0,1,2,3,4 to the console.

#include <iostream>

//Class storing data
class DataStorage
{
public:
    DataStorage()
    {
    }

    ~DataStorage()
    {
        delete x;
    }

    void storeThis(int* xIn, unsigned sizeIn)
    {
        x = xIn;
        size = sizeIn;
    }

    void writeStoredData()
    {
        for (unsigned i=0; i<size; ++i) {
            std::cout << x[i] << std::endl;
        }
    } 

    bool exist(int value)
    {
        for (unsigned i=0; i<size; ++i) {
            if (x = value) {
                return true;
            }
        }
        return true;
    } 

private:
    int *x;
    unsigned size;
}; 

//Stores and writes the stored data
void exercise1()
{
    std::cout << "Exercise 1" << std::endl;
    int x[5] = {0,1,2,3,4};
    DataStorage dataStorage;
    dataStorage.storeThis(x,5);
    dataStorage.writeStoredData();
} 

int main(int argc, char* argv[])
{
    exercise1();
    return 0;
}
 

what simple changes could I make to solve this problem?

The critical problem is that your class 'DataStorage' is not self-contained/breaks encapsulation.

 

The class tries to manage a pointer to integer data:

private:
    int *x;

It also deletes the memory allocated and 'assigned' to 'x' in it's destructor:

    ~DataStorage()
    {
        delete x;
    }

The problem here is that you should only delete (free) memory that has been dynamically allocated using 'new':

int *x = new int;
delete x;

//Or, for a block of (1000) ints:
int *x = new int[1000];
delete [] x;

You should never free automatic memory, which is exactly what you're doing:

int x[5] = {0,1,2,3,4};

'x' is a automatic variable, local to function 'excercise1', it was never dynamically allocated and thus should never be manually freed, it will be gone automatically when it  goes out of scope when the function ends.

 

As said, the main problem is breaking encapsulation or at least dubious ownership:

    void storeThis(int* xIn, unsigned sizeIn)
    {
        x = xIn;
        size = sizeIn;
    }

The function takes a pointer 'xIn', which might've come from anywhere and stores it as 'x':

• There is no guarantee the memory was dynamically allocated as the destructor assumes, it might even be a nullptr.
• There are no clear indications about ownership, who owns the pointer after this function call ? The class or the caller ?
• <pedantic> There is no guarantee sizeIn is correct for the block of data given. </pedantic>

Possible solutions:

• Make the class self contained - The function 'storeThis' could allocate memory using new, after freeing any previously allocated memory ofc, and copy the data over. The destructor will now always be correct when deleting the memory, as it's always dynamically allocated. It also resolves ownership ambiguity, 'DataStorage' is responsible for it's copy of the data, and the caller for it's copy. The class will now also need copy contructor and copy assignment operator - as per the rule of 3. If you're more advanced, a smart pointer (std::unique_ptr) is preferred over manual management of the memory.
• Hold a std::vector as member in stead of a pointer, and have 'storeThis' copy the data to the vector. This is even more preferred, as it has all the pro's of the above solution, but the std::vector will implement copy/move construction/assignment for you for free. You could even take the vector to copy as a parameter, making sure the size is always correct.
• If the class must take ownership of the actual array, 'storeThis' should take a 'std::unique_ptr' as parameter and store that as a private member in stead of the raw pointer. This will make absolutely clear that the class is now the new owner and automate everything. The unique_ptr will clean up itself, the destructor is no longer needed. You will still need copy/move construction/assignment however.
• If the class must simply work with external data without taking any ownership, then your current approach of storing a pointer to the data might be correct (depending on context, resource lifetime must be carefully charted, you do not want the class to outlive the data) but you should not delete the data in the destructor - If you do not take responsibility over a resource you should not delete it.