2d array memory management issue

Question

I have to write a code that gets a string and turns it into an object of a class. Everything is working as expected but I'm unable to deallocate the dynamically allocated 2d array of objects.

I know the issue is within the destructor and the Move assignment operator for the object, I keep getting SIGBRT and EXC_BAD_ACCESS errors when I try to run it.

Below is my Code for the constructor, destructor and move assignment/constructor

//CustomerOrder.cpp
CustomerOrder::CustomerOrder(std::string& 
src):Name(src),Product(),ItemCount(),ItemList(),field_width(){
std::vector<ItemInfo> info;
std::string* tokens[] = { &Name, &Product };
Utilities utils;
size_t next_pos = -1;
bool more = true;

for (auto& i : tokens) {
    if (!more) break;
    *i = utils.extractToken(src, next_pos, more);
}
while (more){
    info.push_back(utils.extractToken(src, next_pos, more));
}
if(!info.empty() && info.back().ItemName.empty()){
    info.pop_back();
}
ItemCount = info.size();


ItemList = new ItemInfo*[ItemCount];
for (int i = 0; i < ItemCount; i++){
    ItemList[i] = new ItemInfo(info.at(i).ItemName);
}
if (utils.getFieldWidth() > field_width){
    field_width = utils.getFieldWidth();
}
}

CustomerOrder::~CustomerOrder(){
for(int i = 0; i<ItemCount;i++){
    delete[] ItemList[i];
}
delete[] ItemList;
}

CustomerOrder::CustomerOrder(CustomerOrder&& src){
*this = std::move(src);
}

CustomerOrder& CustomerOrder::operator=(CustomerOrder&& src){
if(this!= &src){
    delete [] ItemList;
    Name = std::move(src.Name);
    Product = std::move(src.Product);
    ItemCount = std::move(src.ItemCount);
    ItemList = std::move(src.ItemList);
    src.ItemList = nullptr;
}
return *this;
}

And the ItemInfo struct

//ItemInfo struct
struct ItemInfo
{
std::string ItemName;
unsigned int SerialNumber;
bool FillState;

ItemInfo(std::string src) : ItemName(src), SerialNumber(0), 
FillState(false) {};
};

Do yourself a favor and don't use 2D C-arrays. Don't use C-arrays at all. Do use a `std::vector` and access lement `(i, j)` at index `i*width+j`. — YSC, Dec 03 '18 at 16:56

score 0 · Answer 1 · answered Dec 03 '18 at 17:07

0

You are combining "new" with "delete[]". If you use "new" use "delete" if you use "new[]" then use "delete[]" for the thing.

This is your problem there: "delete[] ItemList[i];" it should be "delete ItemList[i];" instead

answered Dec 03 '18 at 17:07

Abdurrahim

2,078
1
17
23

score 0 · Answer 2 · answered Dec 03 '18 at 17:10

This line of your code ItemList[i] = new ItemInfo(info.at(i).ItemName); doesn't allocate a dynamic array, yet this code in your destructor tries to delete it as thought it was a dynamic array.

for(int i = 0; i<ItemCount;i++){
    delete[] ItemList[i];
}

A quick fix would to be to change delete[] to delete. However, it appears as though it would be much easier to simply allocate a single dynamic array. In other words, allocate ItemList as such ItemList = new ItemInfo[ItemCount]; Granted, you would have to change the type, but it makes more sense from what you posted.

Another possible issue is that in your destructor you don't check if the ItemList is a nullptr or actually allocated to anything. To which, your destructor could possibly try to access invalid data. Not only that, but your move operator deletes the ItemList without deleting the data inside of it.

You could make a function to free up the data in ItemList and then call that function from the destructor and move operator.

On a side note, why are you using dynamic 2D arrays when it appears that you know how to use vectors? A vector would handle all of this in a much simpler fashion. For example, the type would be std::vector<std::vector<ItemInfo>>.

[Another good option for a 2D rectangular matrix.](https://stackoverflow.com/a/2076668/4581301) One `vector`. One allocation. Almost no pointer-chasing and generally better cache behaviour. — user4581301, Dec 03 '18 at 17:41

2d array memory management issue

2 Answers2