20

Suppose I have the following snippet.

int main()
{
    int num;
    int* cost;
    while(cin >> num)
    {
        int sum = 0;
        if (num == 0)
          break;

        // Dynamically allocate the array and set to all zeros
        cost = new int [num];
        memset(cost, 0, num);
        for (int i = 0; i < num; i++)
        {
            cin >> cost[i];
            sum += cost[i];
        }
        cout << sum/num;
    }
`  `delete[] cost;
    return 0;
}

Although I can move the delete statement inside the while loop for my code, for understanding purposes, I want to know what happens with the code as it's written. Does C++ allocate different memory spaces each time I use operator new?

Does operator delete only delete the last allocated cost array?

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Ankit Sharma
  • 663
  • 1
  • 5
  • 17
  • 1
    To add to the errors pointed out in the answers, you've go another error there worse than leaking memory: if your `while` executes 0 times, then you will have a `delete[]` without any corresponding `new[]`, so on an uninitialised pointer. That is undefined behavoir, and will probably actually crash your program. (Well, maybe not so dramatic happening one instruction away from the end of your program, but is is not a pretty sight.) – Marc van Leeuwen May 20 '15 at 16:37
  • 1
    You can't allocate an array multiple times. You can allocate multiple arrays. – user253751 May 20 '15 at 19:44
  • 2
    You are not allocating "the" array multiple times. You are allocating "an" array. The name `cost` is the name of a pointer, not the name of an array. – usr May 20 '15 at 22:31

6 Answers6

22

Does C++ allocate different memory spaces each time I use operator new?

Yes.

Does operator delete only delete the last allocated cost array?

Yes.

You've lost the only pointers to the others, so they are irrevocably leaked. To avoid this problem, don't juggle pointers, but use RAII to manage dynamic resources automatically. std::vector would be perfect here (if you actually needed an array at all; your example could just keep reading and re-using a single int).

Community
  • 1
  • 1
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • 2
    @doron: If you're careful that it deletes arrays properly, that's an option, albeit rather weirder than `vector`. – Mike Seymour May 20 '15 at 14:30
  • use something like `unique_ptr cost;`, the `[]` indicates an array. – doron May 20 '15 at 15:04
  • And this is exactly why newer languages only use references instead of pointers. Much harder to leak memory when things obey RAII... or have an actual GC process that sweeps away unreferenced objects in memory. – sfdcfox May 20 '15 at 18:03
  • 2
    @sfdcfox just because something is more foolproof doesn't necessarily mean it's better – chbaker0 May 20 '15 at 20:20
  • @chbaker0 That depends on the class of fool you're dealing with. I certainly appreciate the compiler's ability to prevent my own foolishness, and never manage anything without RAII. – Mike Seymour May 20 '15 at 21:57
  • Me neither. I was disagreeing with the proposition from another comment that GC is better than RAII. – chbaker0 May 20 '15 at 22:02
  • I recently went `vector` for most of my array needs, and haven't looked back. It's so much easier to just `resize` it to the size you want the array to be and let it deal with all the annoying bits. – Carcigenicate May 20 '15 at 23:46
  • @sfdcfox: GC is not problem free; it's notorious for leaking file descriptors, because it's paying attention to memory usage to decide when to run, and you usually run out of file descriptors *long* before you have any memory issues. –  May 21 '15 at 05:18
  • @Hurkyl Problem with RAII, *by itself*, is you still need to call free/delete to cause the resources to be reclaimed. Code such as in OP causes leaks unintentionally. Use GC with RAII, and you can guarantee that those leaks are less likely to be permanent. C# works this way, for example. As objects are GC'd, the Finalize method uses a RAII model to free unmanaged resources like file handles, etc. This reduces the likelihood of memory leaks. Not impossible, certainly, but easier than C++, which requires your code to be more explicit with memory frees. – sfdcfox May 21 '15 at 05:37
  • @sfdcfox: I can't make sense of your response. You seem to be talking about C++ style from 30 years ago, not modern C++ style. And while I'm not familiar with C#, a brief google search shows that its GC also too suffers the exact problem I alluded to. About `Finalize`, MSDN says `The exact time when the finalizer executes is undefined. To ensure deterministic release of resources for instances of your class, implement a Close method ...` That is, you have to explicitly free your resource, whereas, for example, a `std::ifstream` will automatically get closed when it goes out of scope. –  May 21 '15 at 06:33
  • @sfdcfox: "Problem with RAII, by itself, is you still need to call free/delete to cause the resources to be reclaimed" - that's absolute nonsense. The whole point of RAII is that the resource manager's destructor does that for you. You never have to do it yourself, so there's no possibility of memory leaks unless you deliberately do something to circumvent RAII. (Unlike GC, which typically doesn't work deterministically, and only deals with memory and not other resources. Hence the need to add RAII-style hacks to make it work usefully.) – Mike Seymour May 21 '15 at 08:20
  • `char* a = new char[5]; a = new char[10];` You're telling me that in C++, the five bytes allocated to the first array will be cleaned up automatically now? Wasn't that the entire point of this answer, to state there would be a leak? The difference is, if I run that same code in a GC environment, like Java, it will eventually be reclaimed, no memory is leaked. C++ makes no such guarantee, because the variable didn't go out of scope, because the object still exists in memory, lost. – sfdcfox May 21 '15 at 08:24
  • 1
    @sfdcfox: That example doesn't use RAII, so of course it will leak. With RAII, that would be `std::string a(5); a = std::string(10);`. The `string` class uses RAII to deallocate the memory automatically, so there's no leak. The point of the answer was to say you should use RAII, not manual pointer-juggling, to avoid the leak. GC is an alternative to RAII, but not available in standard C++, so I've no idea why you keep going on about it. – Mike Seymour May 21 '15 at 08:27
6

I strongly advise you not to use "C idioms" in a C++ program. Let the std library work for you: that's why it's there. If you want "an array (vector) of n integers," then that's what std::vector is all about, and it "comes with batteries included." You don't have to monkey-around with things such as "setting a maximum size" or "setting it to zero." You simply work with "this thing," whose inner workings you do not [have to ...] care about, knowing that it has already been thoroughly designed and tested.

Furthermore, when you do this, you're working within C++'s existing framework for memory-management. In particular, you're not doing anything "out-of-band" within your own application "that the standard library doesn't know about, and which might (!!) it up."

C++ gives you a very comprehensive library of fast, efficient, robust, well-tested functionality. Leverage it.

Mike Robinson
  • 8,490
  • 5
  • 28
  • 41
5

There is no cost array in your code. In your code cost is a pointer, not an array.

The actual arrays in your code are created by repetitive new int [num] calls. Each call to new creates a new, independent, nameless array object that lives somewhere in dynamic memory. The new array, once created by new[], is accessible through cost pointer. Since the array is nameless, that cost pointer is the only link you have that leads to that nameless array created by new[]. You have no other means to access that nameless array.

And every time you do that cost = new int [num] in your cycle, you are creating a completely new, different array, breaking the link from cost to the previous array and making cost to point to the new one.

Since cost was your only link to the old array, that old array becomes inaccessible. Access to that old array is lost forever. It is becomes a memory leak.

As you correctly stated it yourself, your delete[] expression only deallocates the last array - the one cost ends up pointing to in the end. Of course, this is only true if your code ever executes the cost = new int [num] line. Note that your cycle might terminate without doing a single allocation, in which case you will apply delete[] to an uninitialized (garbage) pointer.

AnT stands with Russia
  • 312,472
  • 42
  • 525
  • 765
3

Yes. So you get a memory leak for each iteration of the loop except the last one.

When you use new, you allocate a new chunk of memory. Assigning the result of the new to a pointer just changes what this pointer points at. It doesn't automatically release the memory this pointer was referencing before (if there was any).

JBL
  • 12,588
  • 4
  • 53
  • 84
3

First off this line is wrong:

memset(cost, 0, num);

It assumes an int is only one char long. More typically it's four. You should use something like this if you want to use memset to initialise the array:

memset(cost, 0, num*sizeof(*cost));

Or better yet dump the memset and use this when you allocate the memory:

cost = new int[num]();

As others have pointed out the delete is incorrectly placed and will leak all memory allocated by its corresponding new except for the last. Move it into the loop.

Steve
  • 1,760
  • 10
  • 18
1

Every time you allocate new memory for the array, the memory that has been previously allocated is leaked. As a rule of thumb you need to free memory as many times as you have allocated.

Ivaylo Strandjev
  • 69,226
  • 18
  • 123
  • 176