Subtle Memory Leak, and is this common practice?

Question

I think I might be creating a memory leak here:

   void commandoptions(){
      cout<< "You have the following options: \n 1). Buy Something.\n  2).Check you balance. \n3). See what you have bought.\n4.) Leave the store.\n\n Enter a number to make your choice:";
      int input;
      cin>>input;
      if (input==1) buy();
      //Continue the list of options.....
      else
      commandoptions(); //MEMORY LEAK IF YOU DELETE THE ELSE STATEMENTS!
   }

inline void buy(){
    //buy something
    commandoptions();
}

Let's say commandoptions has just exectued for the first time the program has been run. The user selects '1', meaning the buy() subroutine is executed by the commandoptions() subroutine.

After buy() executes, it calls commandoptions() again.

Does the first commandoptions() ever return? Or did I just make a memory leak?

If I make a subroutine that does nothing but call itself, it will cause a stackoverflow because the other 'cycles' of that subroutine never exit. Am I doing/close to doing that here?

Note that I used the inline keyword on buy... does that make any difference?

I'd happily ask my professor, he just doesn't seem available. :/

EDIT: I can't believe it didn't occur to me to use a loop, but thanks, I learned something new about my terminology!

Calling your own function recursively is a very bad idea. You want a `while` or `do ... while` loop. — Mats Petersson, May 07 '13 at 15:52
A memory leak is when objects that can no longer be referred to are never deleted, right? — user1833028, May 07 '13 at 15:53
Calling your own function recursively is not inherently bad, in fact that are a lot of problems that are more easily solved using recursion. Ex: algorithms that work on tree type structures. Calling your own function recursively, without some type of end condition... very bad. — MobA11y, May 07 '13 at 15:58
Its' been a few years since I took advanced C++, and I'm really starting to program again, taking another class... I knew something was fishy about this. — user1833028, May 07 '13 at 16:08
See also: [What is a memory leak?](http://stackoverflow.com/q/3373854/78845) and [Recursion](http://stackoverflow.com/q/16423321/78845#comment23557429_16423321) — johnsyweb, May 07 '13 at 19:41
@DeadMG: Yes, for solving problems that are in themselves recursive. This is clearly a "main menu -> functins" type piece of code, and it's not typically a recursively solved problem. I'm pretty sure that IN THIS CASE recursion is very much wrong wrong. — Mats Petersson, May 08 '13 at 08:52

score 7 · Answer 1 · answered May 07 '13 at 15:55

7

A memory leak is where you have allocated some memory using new like so:

char* memory = new char[100]; //allocate 100 bytes

and then you forget, after using this memory to delete the memory

delete[] memory; //return used memory back to system.

If you forget to delete then you are leaving this memory as in-use while your program is running and cannot be reused for something else. Seeing that memory is a limited resource, doing this millions of times for example, without the program terminating, would end you with no memory left to use.

This is why we clean up after ourselves.

In C++ you'd use an idiom like RAII to prevent memory leaks.

class RAII
{
public:
    RAII() { memory = new char[100]; }
    ~RAII() { delete[] memory }
    //other functions doing stuff
private:
   char* memory;
};

Now you can use this RAII class, as so

{ // some scope
RAII r; // allocate some memory

//do stuff with r

} // end of scope destroys r and calls destructor, deleting memory

Your code doesn't show any memory allocations, therefore has no visible leak.

Your code does seem to have endless recursion, without a base case that will terminate the recursion.

answered May 07 '13 at 15:55

Tony The Lion

61,704
67
242
415

You only need to call the delete[] on memory if memory is a pointer to something external right... char memory would've been destroyed with the class, sir? – user1833028 May 07 '13 at 16:07
He mixed a few concepts together. You only have to call delete on memory allocated with "new". However, "delete[]" vs simply "delete" is subtly different. You only call "delete[]" when you allocated an array using new. And you must do so, before the memory adress your pointer is pointing to changes, or keep another reference somewhere else to that point in memory. Example somePointer = new object. SomeTempPointer = somePOinter. You should now only cale delete on one of these pointers, and one of them can go out of scope without creating a leak. – MobA11y May 07 '13 at 16:08
That sounds so familiar.... dang.... Thanks, I'll have to review that! .... I normally use straight C... – user1833028 May 07 '13 at 16:11
6

Actually, you _never_ use `new[]`; you use `std::vector`. In which case, there's no memory leak. – James Kanze May 07 '13 at 16:21
Can you downvote comments??? Or do I just have to say that James is completely incorrect. There are plenty of times when using new[] is much better than using std::vector. Char arrays being an obvious one, but there are plenty of others. – MobA11y May 07 '13 at 16:38
1

@ChrisCM you make me want to cry. @JamesKanze is very correct in what he is saying. In my answer I was merely trying to explain a memory leak and its mechanics. You should really use `std::vector` or some such construct over `new[]` – Tony The Lion May 07 '13 at 16:42
This is entirely false. There are a lot of reasons to use char arrays. There are entire libraries of pre defined functions that concatenate, manipulate, and lexigraphically compare character arrays. Vectors involve a lot of overhead. Often place in character arrays you are storing a word that will never change, which does not need the overhead of a vector... shall I keep going??? – MobA11y May 07 '13 at 16:45
3

Yes. Keep going. Tell us about the overhead. – R. Martinho Fernandes May 07 '13 at 16:53
My point was simply never say never. Yes the overhead of a vector is mostly overblown. You see the occasional const char*, it's a convenient solution for the occasional string manipulation issue, where other solutions are sluggish, passing around a pointer to the same string is fast. On the other hand, I don't believe I have ever seen "const std::vector" in anyone's C++ file... ever. Besides, if you have an array of characters you probably want to use std::string regardless, though const char*, even with c++ strings, still has its uses, and you can't say "always" use strings either. – MobA11y May 07 '13 at 17:32
I still don't know what it is that is overhead or overblown or overwhatever, but I am getting curiouser and curiouser. – R. Martinho Fernandes May 07 '13 at 18:28
@ChrisCM weren't you going to tell us about the overhead of a vector? What overhead does it have that `new char[100]` does not? – jalf May 07 '13 at 18:38
I was able to put together a very simple example in about 2 minutes, where vectors were significantly less performant than arrays, I outlined it in my answer. I'm sure I could come up with more. – MobA11y May 07 '13 at 19:26
http://stackoverflow.com/questions/2740020/c-stl-array-vs-vector-raw-element-accessing-performance This is a really good discussion on the issue. – MobA11y May 07 '13 at 19:37
6

@ChrisCM: That's a red herring. The discussion *there* is about `char on_stack[N];` vs `std::vector`. The discussion *here* is about `char* on_heap = new char[N];` vs `std::vector`. – Xeo May 07 '13 at 23:57

Sander De Dycker · Answer 2 · 2013-05-07T15:58:24.767

This is basically a recursion without a base case. So, the recursion will never end (until you run out of stack space that is).

For what you're trying to do, you're better off using a loop, rather than recursion.

And to answer your specific questions :

No, commandoptions never returns.
If you use a very broad definition of a memory leak, then this is a memory leak, since you're creating stack frames without ever removing them again. Most people wouldn't label it as such though (including me).
Yes, you are indeed gonna cause a stack overflow eventually.
The inline keyword won't make a difference in this.

score 5 · Answer 3 · answered May 07 '13 at 15:54

5

Inline keyword won't cause a memory leak.

If this is all the code you have, there shouldn't be a memory leak. It does look like you have infinite recursion though. If the user types '1' then commandoptions() gets called again inside of buy(). Suppose they type '1' in that one. Repeat ad infinum, you then eventually crash because the stack got too deep.

Even if the user doesn't type '1', you still call commandoptions() again inside of commandoptions() at the else, which will have the exact same result -- a crash because of infinite recursion.

I don't see a memory leak with the exact code given however.

answered May 07 '13 at 15:54

inetknght

4,300
1
26
52

Will the inline keyword prevent the stack crash on account of causing it to execute as if it were part of the subroutine? That's what I'd like to know now... not sure how it behaves, exactly... – user1833028 May 07 '13 at 15:56
Long answer: `inline` keyword allows the compiler to put the function inside of every call to it. It generally won't work in a recursive function because the function would be added inside of itself, again ad infinum -- the function would be infinite in length because it's always calling itself. – inetknght May 07 '13 at 15:59

score 2 · Answer 4 · answered May 07 '13 at 15:54

2

This is not about memory leak, you are making infinite calls to commandoptions function no matter what the value of input is, which will result in stack crash. You need some exit point in your commandoptions function.

answered May 07 '13 at 15:54

taocp

23,276
10
49
62

score 1 · Answer 5 · answered May 07 '13 at 15:54

There is no memory leak here. What does happen (at least it looks that way in that butchered code snippet of yours) is that you get into an infinite loop. You might run out of stack space if tail call optimization doesn't kick in or isn't supported by your compiler (it's a bit hard to see whether or not your calls actually are in tail position though).

Subtle Memory Leak, and is this common practice?

5 Answers5

Linked