-1

I have following c++ code (psuedo)

while(!fileEnd)
{
    rectD *rD = new rectD();
    symbol *sb = new symbol(rD);
    
    while( true )
    {
        if(closing brace)
        {
            sb->rD->isSymbol = true;
            sb->shapes[currentSymName] = *(sb->rD); // shapes is a std::map
            allSymbol[currentSymName] = sb; // allSymbol is a std::map
            break;
        }
        else
        {
            if(condition 1)
               sb->fun1();
            if(condition 2)
               sb->fun2();
        }
    }
}     
   

Now in rest of the program I use shapes and allSymbol map.

Here I want to ask,

Is new keyword necessary here ?

I know when we create object using new then it is created on heap memory and without using new, object is created on satck memory.

Heap memory lives until we delete it and stack memory gets deleted once function scope gets end.
In above code, Once the scope of above function ends I am no longer using *rD and *sb. But in through out program I am using std::map , shapes and allSymbols which contains *sb and *rD.

So I am confused, whether should I use new keyword or not ?
And if I have to use it, where should I release the memory ( using delete keyword) to avoid memory leak ?

tushar
  • 313
  • 4
  • 10
  • To answer your first question: yes. In C++ objects are created all the time without the `new` keyword. One question per Stackoverflow question, please. – Sam Varshavchik Sep 13 '22 at 11:00
  • 2
    Besides for polymorphism, you seldom need pointers and explicit `new` or `delete` in modern C++. – Some programmer dude Sep 13 '22 at 11:01
  • 1
    [Why should C++ programmers minimize use of 'new'?](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new). You should never use raw `new`. The better question to ask is: Should you use dynamic allocations? And I suppose the answer is: No. – 463035818_is_not_an_ai Sep 13 '22 at 11:03
  • You can consider to change `allSymbol` map to hold `symbol` objects (rather than `symbol*`). This will enable you to use `symbol sb` (without a pointer and without `new`). – wohlstad Sep 13 '22 at 11:21
  • `std::map>` is most likely what you want. OTOH, depending on whether `symbol` is copyable and/or moveable, you could also have just a `std::map` and then (e.g.) `allSymbol.try_emplace(currentSymName, rD);` or the like. That gives you fewer allocations, less pointer jumping etc. In any case, the keyword to look for is **ownership**. That’s what decides what type of (smart) pointer / allocation / encapsulation to use. – Andrej Podzimek Sep 13 '22 at 12:55

2 Answers2

0

" I am using std::map, shapes and allSymbols which contains *sb and *rD"

No, they don't. They contain a copy of *rD, and the pointer sb and

The original objecs *rD is leaked since you forgot delete. But this is only a problem because you started with new. Had you just created standard objects in the loop, your shapes would still have contained copies, but now the automatic cleanup would have destroyed the originals instead of leaking them. The code is incomplete, so we can't tell if you delete the pointers in allSymbol, but this is definitely necessary. This too would be easier if you stored the symbol directly. Also, sometimes you don't store sb, and then it's certainly leaked.

As an optimization, you can look into moving objects, but you should first learn the basics.

MSalters
  • 173,980
  • 10
  • 155
  • 350
  • According to this line: `allSymbol[currentSymName] = sb` it seems like the map is holding pointers to `symbol` (not `symbol`) so the copy is actually of the pointer not the object. – wohlstad Sep 13 '22 at 11:26
  • @wohlstad: Good point - I didn't see a declaration and just relied on the text. – MSalters Sep 13 '22 at 11:33
  • After executing above loop , If I store let's say at 100th location a symbol and it stored in map `allSymbol`. After above function scope gets ended, that memory location 100 will be claimed by OS. To avoid that, we used `new` keyword. Now that memory location 100th will not be claimed by OS though function scope is over. But now it is our responsibility to release memory location 100th. So now let's say, I have defined `allSymbol` in `ABC` class. Then in `ABC` class's destructor, I need to delete memory location 100th. Is this understanding correct ? – tushar Sep 13 '22 at 12:06
0

If you are using new/delete and only need these variables inside the loop, then you will have to delete as shown.

while(!fileEnd)
{
    rectD *rD = new rectD();
    symbol *sb = new symbol(rD);
    
    while( true )
    {
        if(closing brace)
        {...}
        else
        {...}
    }
    // Free allocated memory: here 
    delete rD;
    delete sb;
}     

As to whether you should use new/delete here, you probably shouldn't in this case, because the program will be slower, because it is asking the operating system for memory every iteration of the loop while it could have just as easily reused old memory and reset its content.

People argue that you shouldn't use new/delete EVER, but what they are advising is from a developer point of view and in the context of code production, if you are learning feel free to explore these constructs you will learn a lot about memory and how it works and you will validate for yourself why it is advisable to avoid using them in production or if you are open minded enough when it is best to use them.

There are various places where you might need dynamic memory, but most of the time there is some std:: template(like string, vector, map, unique_ptr, etc) that does the job, while also automatically releasing memory.

Hope I helped.

vonamedog
  • 50
  • 5