0

upon trying to delete the pointer obs I get a crash (im suspecting garbage data) if I run this and enter 1 and fill out the questions, the array increments right, and is able to delete. but upon running this and entering 1 twice (both filling out data). I get a crash. the array is incremented, and holds both sets of data that I have entered. also I am unable to use vectors, due to this being a school project.

#include <fstream> 
#include <iostream> 
#include <iomanip> 
#include <string>

using namespace std; 

#include "fileStuff.h"

bool menu();
bool menuOptions(int option);
void fileIO();
void listAll(interact * obs, int arryO);
interact addOne(interact * obs, int size); 
interact subOne(interact * obs, int size);

int main() 
{ 

    bool isRunning = true;
    while (isRunning)
    {
        isRunning = menu();
    }
    return 0; 
}
interact addOne(interact * obs, int size)
{
    interact *temp = new interact[size];
    cout << temp[0].getBagId() << " from method." << endl;
    for (int i = 0; i < size ; i++)
    {   
        cout << temp[i].getBagId() << endl;

        temp[i] = obs[i];

    }

    return *temp;
}
interact subOne(interact * obs, int size)
{
    return *obs;
}
bool menu()
{
    int option = 0;
    cout << "1: add new backpack. " << endl
        << "2: delete a backpack "<< endl
        << "3: sort ascending by id " << endl
        << "4: sort descending by id " << endl
        << "5: list all backpacks " << endl
        << "6: quit" << endl;
    cin >> option;
    return menuOptions(option);
}

bool menuOptions(int option)
{
    static int arrayO = 0;
    cout << arrayO << endl;
    static interact *obs = new interact[arrayO];
    fileStuff test;
    int tempBagId = 0, tempInvSpaces = 0, tempAmtOfItemsInInv = 0;
    double tempInvMaxWeight = 0.0;
    string tempBagType, tempBagCondish;
    int t = 0 ;
    int i = 0;
    switch (option)
    {
    case 1:
        cout << "bagId? ";
        cin >> tempBagId;
        cout << "How many inv spaces? ";
        cin >> tempInvSpaces;
        cout << "How much weight can the bag hold? ";
        cin >> tempInvMaxWeight;

        obs[arrayO].setBagId(tempBagId);
        obs[arrayO].setInvSpaces(tempInvSpaces);
        obs[arrayO].setInvMaxWeight(tempInvMaxWeight);

        //      test.writeToFile(obs, arrayO);
        cout << "all stored" << endl;

        arrayO++;
        *obs = addOne(obs, arrayO);
        cout << obs[0].getBagId() << "ERE" << endl;
        break;

    case 5:
        //list all
        listAll(obs, arrayO);
        break;

    case 6:
        obs = NULL;
        delete obs;
        //      exit(0);
        return false;
        break;
    default:
        break;
    }
}


void listAll(interact * obs, int arryO)
{
    int i = 0;
    for (i; i <= arryO; i++)
    {
        cout << (obs + i)->getBagId() << endl;
        cout << (obs + i)->getInvSpaces() << endl;
        cout << (obs + i)->getInvMaxWeight() << endl;
    }
}
hurnhu
  • 888
  • 2
  • 11
  • 30
  • 1
    possible duplicate of [The Definitive C++ Book Guide and List](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) – Bartek Banachewicz Feb 04 '15 at 13:39
  • 1
    This code should be shortened, and all of the unnecessary bits removed. – Bartek Banachewicz Feb 04 '15 at 13:39
  • `addOne` returns a single `interact` object, but you dereference a dynamically allocated array to return that *single* instance. Also, I'm not seeing any kind of `delete` here (you leak memory!) – crashmstr Feb 04 '15 at 13:42
  • Please provide the code relevant to the problem or error you are getting. – Prerak Sola Feb 04 '15 at 13:43
  • 1
    In menuOptions you are allocating an array of size 0. Then you try to put values into that array. Then you try to expand it by replacing the pointer with a new array. – drescherjm Feb 04 '15 at 13:44
  • Your `addOne` does`return *obs;` (i.e. `return obs[0];`) in a very roundabout way. – molbdnilo Feb 04 '15 at 13:44
  • 1
    You set `obs = NULL` and after that you call `delete[] obs`. That is not freeing the memory which **was** pointed to by `obs`. – a_guest Feb 04 '15 at 13:47

2 Answers2

5

You allocate obs with

static interact *obs = new interact[arrayO];

But deallocate it with:

delete obs;

Use delete[] instead.


Also you're clearing obs before you delete it, which never has a chance of working properly.


As pointed out by @piwi, the second time you don't initialize it, because it's a static variable.


Frankly, though, the purpose of dynamic allocation is unclear for me here. Aside from the fact that it shouldn't be a static in the first place, but a class instance instead, you can just use value semantics and std::array.

Bartek Banachewicz
  • 38,596
  • 7
  • 91
  • 135
  • I have tried changing delete obs; to delete[] obs; and am still getting crash – hurnhu Feb 04 '15 at 13:41
  • 2
    Also, I think it crashes the second time because `obs` is deleted and not instantiated again since it's a static variable. – piwi Feb 04 '15 at 13:45
3

Your code has multiple errors. The memory corruption starts, though, when you allocate an array of size array0 and then access obs[array0]. The index is one too many already, and as array0 gets bigger it just goes further off the end of your allocated array.

Also, your addOne function is completely wrong. It allocates a new, larger array, carefully copies the data into it, then returns the contents of the first element of the new array (not the new array itself). The new array itself is just discarded (leaked), since you don't return the pointer to it.

As to the actual delete, Bartek is of course correct that it should be delete[], but in any case you only ever delete a null pointer:

obs = NULL;
delete obs;

This doesn't do anything to the array that obs used to point to before you assigned a null pointer to obs.

Steve Jessop
  • 273,490
  • 39
  • 460
  • 699
  • sadly I am not allowed to user vectors due to school project – hurnhu Feb 04 '15 at 13:44
  • 3
    I will never understand why school C++ courses do not teach C++. – Lightness Races in Orbit Feb 04 '15 at 13:47
  • @MichaelLapan Copy the sources of the `vector` from your standard library or other widely available implementation, paste them in your code, and use them (as long as you understand how `vector` works, it shouldn't be a problem to explain if anyone asks). The requirement is nonsensical. – Bartek Banachewicz Feb 04 '15 at 13:47
  • 1
    @BartekBanachewicz: that would almost certainly be considered plagiarism (in the academic sense, despite that the license on the standard library code permits it). – Steve Jessop Feb 04 '15 at 13:48
  • @BartekBanachewicz: That's ... um, sorry, but that's a really stupid suggestion and you know it. – Lightness Races in Orbit Feb 04 '15 at 13:48
  • @LightnessRacesinOrbit: certainly I don't understand why they teach it in such a peculiar order. It would make more sense to me to first learn how to use `vector`, then learn how to implement it (or something like it). But this exercise apparently asks someone to implement and use something like `vector` all in one step. So the student is made to do something difficult, then informed their labour was pointless. – Steve Jessop Feb 04 '15 at 13:52
  • If `unique_ptr` is allowed, it can simplify the implementation vastly. – Bartek Banachewicz Feb 04 '15 at 13:54