0

I've a fairly complex application written in c++. I've a class called OrderBook. I need to create an array of OrderBook objects dynamically, so what i've done is,

OrderBook* pOrderBooks; // In header file

At runtime, I create the array as 

p_OrderBooks = new OrderBook[n]; // n is an integer initialized at run time

The program works fine. But when I try to delete the array (as I need to create a new array pointed by pOrderBooks) the program crashes. This is how i delete it.

delete[] p_OrderBooks;

I've made sure that the crash happen exactly due to that line. So what i'm currently doing is reinitializing the pointer without deleting the previously allocated memory. 

//delete[] p_OrderBooks; // <- crash happens here
p_OrderBooks = new OrderBook[k]; // for some 'k'

But it's bad since there'll be a memory leak. I'd like to know how to properly free the memory before re-pointing to the new array.

Anubis
  • 6,995
  • 14
  • 56
  • 87
  • 7
    Why not use `std::vector` and avoid the need to juggle pointers in the first place? You're probably deleting the array twice, which is easy to do when mucking around with low-level memory allocation. – Mike Seymour May 01 '14 at 14:44
  • If you replace the arrays with `std::vector`s, do you get the same problem? – Eric Finn May 01 '14 at 14:44
  • 2
    probably it's a typo, but you're not creating and deleting the same pointer... name is different. – Jepessen May 01 '14 at 14:45
  • Unless problem is `pOrderBooks` vs `p_OrderBooks`, question does not reveal what the problem is. Add more code, preferably an [MVCE](http://stackoverflow.com/help/mcve). – hyde May 01 '14 at 14:49
  • I'm experimenting with performance. So i'm trying to do this without generic SLT data structures. And sorry about the typo in the post, i've fixed it. – Anubis May 01 '14 at 14:53
  • Any extra performance you get from using raw pointers will be worth far less than the code clarity that will come from using `std::vector`. The standard library containers are very efficient. – Rob K May 01 '14 at 14:55
  • 1
    @Anubis: Then you might consider `std::unique_ptr`, which should be as efficient as a dumb pointer while still managing the memory deallocation correctly. (As with a dumb pointer, you'll need to keep track of the array size separately; but you'll avoid `vector`'s overhead for maintaining the capacity, and value-initialising elements when it grows). – Mike Seymour May 01 '14 at 14:56
  • `delete[] p_OrderBooks;` this statement is correct, probably you are doing something wrong in `OrderBook` class. verify once whether same is not shared by other objects and it is not getting deleted twice. – rajenpandit May 01 '14 at 14:56
  • 1
    @Anubis - `I'm experimenting with performance.` You don't even have a working program. You can't optimize a broken program. You "experiment with performance" *after* you have developed a program that works correctly. Using the STL containers or smart pointers should be done to ensure a working program. Then you profile your code to determine what is slow. Then and only then do you fine tune (if you need to fine tune). – PaulMcKenzie May 01 '14 at 15:28
  • `I've a fairly complex application written in c++. ` Then this is a great time to take the complex application and learn to *reduce* the complexity by introducing higher-level constructs. Making it even more complex for no reason is counterproductive. Rather than guessing what the problem is (you really didn't post anything except for a call to `new[]` and `delete[]`), take the advice to use std::vector or smart pointer. – PaulMcKenzie May 01 '14 at 15:42
  • I found the issue. I'm passing a pointer to a object created in the base class to `OrderBook` objects. In the `OrderBook`'s distructor i'm trying to delete that pointer, which is still being used in the base class. That's all my bad. Thanks everyone! still not experienced enough with c++ – Anubis May 01 '14 at 16:21

4 Answers4

1

You are allocating p_OrderBooks but deleting pOrderBooks

If that's just a simple typo in your post, then it is likely that you are overrunning the bounds of this array, writing to elements past the beginning or end, therefore corrupting the heap so it crashes when you try to delete it.

Zebra North
  • 11,412
  • 7
  • 37
  • 49
  • Sorry, i've fixed the typo. How can i be overrunning as you suggested?? the program works fine after commenting that single line?? – Anubis May 01 '14 at 14:50
  • You say it's a complicated program, so it's possible that you are miscalculating the array index somewhere. For example you allocate enough space for 10 items, but then you write 12 items. That kind of thing will cause the program to crash like this. It's also possible that some entirely unrelated part of the program can be making a mistake like that, and it just happens to end up scribbling over the pOrderBooks allocation. It can be difficult to track down. Data breakpoints can be useful in your debugger. – Zebra North May 01 '14 at 15:50
1

Is it possible that one or more of your destructors for OrderBook is throwing an exception out of the destructor? If so (typically considered bad) and if it is not handled will crash your application.

Community
  • 1
  • 1
brader24
  • 485
  • 2
  • 10
1

If you are doing something like this:

OrderBook* p_OrderBooks;

int n;

p_OrderBooks = new OrderBook[n]; // here n contains garbage

cin>>n

delete[] p_OrderBooks;

Here n can be any garbage value, we don't know its size ,and perhaps we start accessing memory that we don't own. It could be problematic. You should take input first

cin>>n

p_OrderBooks = new OrderBook[n];

Community
  • 1
  • 1
Bramsh
  • 389
  • 1
  • 2
  • 14
  • Why would that matter? – Lilshieste May 01 '14 at 15:50
  • 1
    why won't that matter? – Bramsh May 01 '14 at 17:17
  • 1
    Sorry, misread your point. (Didn't downvote, though.) I don't believe it would make a difference, though, because even though `n` is uninitialized, it's still a valid `int`. If `n` happens to be 0, the `new[]` operator [still returns a valid non-null pointer](http://www.cplusplus.com/reference/new/operator%20new%5b%5d/). If `n` happens to be positive, the `new[]` keyword dynamically allocates an array whose size is unknown to *us*. (`n` can't be negative, since it has to be cast to a `size_t` somewhere before being passed to the `new[]` operator.) So if `new[]` succeeds, `delete[]` will too. – Lilshieste May 01 '14 at 17:39
  • 1
    suppose n is any garbage value, and we started accessing that memory but we don't know its size , then perhaps we start accessing memory that we don't own. – Bramsh May 01 '14 at 18:01
  • 1
    Sure, but that's *accessing* the memory; not `delete`ing it. Your answer currently implies that trying to call `delete[]` on the array without initializing `n` is problematic. If this isn't the point you're trying to make, then it would be helpful if you edited the code in your answer to demonstrate the problematic array access. – Lilshieste May 01 '14 at 18:08
  • ok good advice. but once it happened that because of this it caused my program to crash when I called delete[]. – Bramsh May 01 '14 at 18:10
1

I found the issue. I'm passing a pointer to an object created in the base class to OrderBook objects. 

Server* p_Server = new Server(); // Some class
...
pOrderbook[i]->SetServer(p_Server) // <- for i=[0:99]

Passed p_Server is stored in OrderBook objects as p_ServerBase (say). 

Server* p_ServerBase; // <- in OrderBook.h
...
OrderBook::SetServer(Server* pServer)
{
    p_ServerBase = pServer;
}

Then in the OrderBook's distructor i'm trying to delete that, p_ServerBase, which is still being used in the base class. 

...
~OrderBook()
{
    delete p_ServerBase;
}

Haven't had that experiance before. I' won't do that again :)

Anubis
  • 6,995
  • 14
  • 56
  • 87