CODEWITHSUNDEEP
c++classmemory-management
Anan
Anan

Reputation: 181

Memory management issue in c++

A very simple code with a weird issue. The code goes through fine but I can't seem to get the desired output. My getStock() and getQuantity() functions don't seem to work. When I debug the code, it says 'error reading the memory'. When the execution reaches s.dispP() the code crashes unexpectedly. Can't seem to find a solution for it. Kindly help. Thank you.

    #include<iostream>
    #include<conio.h>
    using namespace std;
    class Sale
    {
        class SaleItem
        {
            int stock, quantity;
        public:

            SaleItem(int pstock, int pquantity) : stock(pstock), quantity(pquantity)
            {

            }
            int getStock()
            {
                return stock;
            }
            int getQuantity()
            {
                return quantity;
            }
        };

        int sstock, squantity;
    public:
        SaleItem *si;
        void addP()
        {
            cout << "Enter Stock: ";
            cin >> sstock;
            cout << "Enter Quantity: ";
            cin >> squantity;
            SaleItem *si = new SaleItem(sstock, squantity);
        }

        void dispP()
        {

            cout << si->getStock() << endl << si->getQuantity();
        }
    };

    void main()
    {
        Sale s;
        s.addP();
        s.dispP();
        _getch();
    }

Upvotes: 1

Views: 146

Answers (6)

Curious
Curious

Reputation: 21510

The error comes from the following method:

void addP() {

        cout << "Enter Stock: ";
        cin >> sstock;
        cout << "Enter Quantity: ";
        cin >> squantity;
        SaleItem *si = new SaleItem(sstock, squantity);
}

Here the si is just a local variable and not the member variable you are thinking it is. To fix the issue, just prepend the si with a this-> or just use it without the this pointer.

void addP() {

        cout << "Enter Stock: ";
        cin >> sstock;
        cout << "Enter Quantity: ";
        cin >> squantity;
        this->si = new SaleItem(sstock, squantity);
}

Alternative is to use a naming convention for member variables, e.g. prefix m_, _ or suffix _.

Although the correct modern C++ approach here is to not use raw pointers at all. Any memory you allocate with new must have delete called on it. And you have not called delete to deallocate the memory you allocated, and this causes a memory leak.

The modern C++ solution is to use std::unique_ptrs instead to automate memory management. 

public:
    std::unique_ptr<SaleItem> si;
    void addP()
    {
        cout << "Enter Stock: ";
        cin >> sstock;
        cout << "Enter Quantity: ";
        cin >> squantity;
        this->si = std::make_unique<SaleItem>(sstock, squantity);
    }

    void dispP()
    {

        cout << si->getStock() << endl << si->getQuantity();
    }

Note that you might not need to use smart pointers here at all. Simple objects might do. Have the knowledge of the options available at your disposal and use the best one :)

Upvotes: 6

Silvia
Silvia

Reputation: 348

I am not very sure why you want to use si as a pointer to SaleItem? Maybe I am missing something, but it doesn't seem to me necessary, unless you are trying to create a list of SaleItems (then I don't think you are implementing the class correctly, see below).

In any case, with that structure I would just go for 

    public:
          SaleItem si(0,0);
          void addP()
          {
                cout << "Enter Stock: ";
                cin >> sstock;
                si.stock=sstock
                cout << "Enter Quantity: ";
                cin >> squantity;
                si.quantity=squantity;
          }

          void dispP()
          {

             cout << si.getStock() << endl << si.getQuantity();
          }

However, if what you are trying to create is a class Sale that is a list of SaleItems then you need to approach things differently. In that case yes, you would have a pointer to an object SaleItem. I would understand that si is an array of elements of the type SaleItem. You then need to allocate memory for the array when you create it, which implies that you know beforehand the maximun number of elements this array will have. Then you can have an index and each time you add a new SaleItem to the array you do it according to this index, being careful not to go beyond the allocated maximum number of items.

If what you are looking to implement is a dynamic data structure that grows as you add elements, then depending on what particular one you choose to implement you will need to make use of pointers. For a example, you could have a pointer to the next SaleItem and a pointer to the previous one. Or alternatively, the pointer to the next SaleItem could be within the SaleItem definition. The point is that you will have to link them somewhere, somehow. If this is the case then I recommend you do some reading on data structures. You can try

http://www.cprogramming.com/algorithms-and-data-structures.html

https://www.tutorialspoint.com/cplusplus/cpp_data_structures.htm

https://www.tutorialspoint.com/data_structures_algorithms/data_structures_basics.htm

If that is what you are after, I hope that helps :)

Upvotes: 0

Abhay Bhave
Abhay Bhave

Reputation: 44

    SaleItem *si;
    void addP()
    {
        cout << "Enter Stock: ";
        cin >> sstock;
        cout << "Enter Quantity: ";
        cin >> squantity;
        SaleItem *si = new SaleItem(sstock, squantity);
    }

At the last SaleItem *si = new SaleItem(sstock, squantity); you are creating another, function local SaleItem* si which holds the newly created object's address, and then it immediately goes out of scope.

Instead, you want your member si to be assigned with it, so another function can use it.

So, instead of declaring another si, just refer the member si.

    SaleItem *si;
    void addP()
    {
        cout << "Enter Stock: ";
        cin >> sstock;
        cout << "Enter Quantity: ";
        cin >> squantity;
        this->si = new SaleItem(sstock, squantity);
    }

P.S. If you are initializing the member si from within a function, and accessing it from yet another function, you can't be sure about the order of calls.

To save yourself from later headache because of initial garbage address in pointer si causing segfaults, I would suggest to have it initialized to nullptr :)

Upvotes: 0

Nandin Borjigin
Nandin Borjigin

Reputation: 2154

You declared a local variable si in the addP() method which shadows the member variable si of the class Sale. Thereby the member variable si is not initialized and your subsequent reference to it causes a crash.

Upvotes: 0

R Sahu
R Sahu

Reputation: 206577

The line

SaleItem *si = new SaleItem(sstock, squantity);

introduces a local variable named si. It does not set the value of the member variable of the class. As a consequence, the member variable si remains uninitialized. Accessing such a variable causes undefined behavior.

You can use

si = new SaleItem(sstock, squantity);

to remove the particular problem you are facing but realize that your class is very fragile.

  1. The member variables sstock and squantity seem to be intended for SaleItem but they are declared outside of that class. It's not clear whether that was from an error in copying and pasting code from you computer to the post, or the error exists on your computer too.

  2. It's always a good idea to initialize all member variables of a class in the constructor. si can be initialized to nullptr in the constructor of the class.

  3. You haven't shown why you need to use a pointer. If your class needs one object, use an object. If it needs a list of objects, use a std::vector of objects.

  4. If, for some reason, you need to store a pointer in your class, you need to be aware of The Rule of Three and make sure to update your class accordingly.

Upvotes: 1

Matteo Italia
Matteo Italia

Reputation: 126787

Here

SaleItem *si = new SaleItem(sstock, squantity);

you are not assigning the result of the new expression to the si field; instead, you created a local variable si (that shadows the field that has the same name) and initialized it with the result of the new expression.

The si field is remains uninitialized, and thus when you later try to use it you get a crash (actually, you are lucky, an uninitialized pointer may silently appear to work and overwrite unrelated memory).

To fix this, you have to change the new variable definition in an assignment; so, that line simply becomes 

si = new SaleItem(sstock, squantity);

Notice that your class is leaking memory, as it calls new without a corresponding delete; the immediate fix here would be to use a smart pointer such as unique_ptr, but, unless you need a pointer for some other reason, here you should just have SaleItem as a "regular" (non-pointer) field inside Sale and forget about all memory management issues.

Upvotes: 1

Related Questions