1

This is an extension of this question . I have searched on the web but couldn't find a satisfactory answer.

I have a class A which defines a vector that contains a number of instances of class Region. The memory handling for these Region instances need to be managed by Class A

My questions are:

1. In the following code snippet, do I need to explicitly delete these instances in the desctuctor of class A?

2. Should I create instances as new Region(i) ? Is it better over the first option (not using new) Class A is quite a large object. How should I determine whether to use new or not?

3. If Answer to #2 is yes, how should I delete these instances in the destructor ? delete Region(i) (in a for loop) or delete [] reg?? I am guessing former way is the correct way, but want to confirm.

4. Other than "Region instances are not deleted" , do you see any obvious memory leak... especially in the construction of Region object in A's constructor?

class Region{
   public:
      Region(int num);
      int number;
      std::vector <elemt*> elements;   
      int info;
}

class A{
public:
    std::vector<Region> reg;
    const int numOfRegions = 100;
}

A::A(){
    int i;
    for ( i=0; i<numOfRegions; i++){
        reg.push_back(Region(i));
      } 
}

A::~A(){
  // How should the elements within vector Region be deleted??
  // Should I use "new" to allocate memory to instances of Region()
}

UPDATE: A BIG Thanks to all who answered!

Community
  • 1
  • 1
memC
  • 1,005
  • 2
  • 14
  • 23

4 Answers4

3

  1. No. std::vector stores the Regions as objects and calls the necessary (de)constructors when elements are added/removed/overridden.

  2. No. Only if you declare your vector as std::vector<Region *> reg;. You have to do that if Region is polymorphic or the copy/assignment constructors are expensive (vector copies elements around often).

  3. If you store them as pointers you need to iterate over the reg and delete each element with delete ptr;.

  4. Depending on how you create instances of the elemt-class you may want to add a destructor to Region to destroy these instances.

ebo
  • 8,985
  • 3
  • 31
  • 37
3

In the following code snippet, do I need to explicitly delete these instances in the desctuctor of class A?

No - they will be automatically deleted for you - if you don't create it with new, you don't delete it.

Should I create instances as new Region(i) ? Is it better over the first option (not using new) Class A is quite a large object. How should I determine whether to use new or not?

The size of A is not an issue. There is no clear answer to this question but the general rule is that if you don't have to create objects dynamically using new, then don't do so.

If Answer to #2 is yes, how should I delete these instances in the destructor ? delete Region(i) (in a for loop) or delete [] reg?? I am guessing former way is the correct way, but want to confirm.

If you created them using new, you would delete them in a for loop as:

 delete reg[i];

reg would have to be a vector of pointers of course. a better option would be to use a vector of smart pointers.

Other than "Region instances are not deleted" , do you see any obvious memory leak.. especially in the construction of Region object in A's constructor?

They are deleted - just not by you. This is known as RAII and is very, very good C++ practice - you should use RAII whenever possible.

  • @Neil: Excellent answer. You guys are very helpful (and are always there for help). I just have one small doubt, In the above code, I illustarated that the Region instances are added within the constructor of class A. But, if I am to populate this list, at the run time, by calling a method on class A::addRegions(int num) , should I use "new" in this case? or should I just stick with my current code? – memC Feb 10 '10 at 12:08
  • 1
    @memC: Its the vector which determines how you should add items to it. If in the constructor you do: `reg.push_back(Region(i))` then you need something similar in a method like `A::addRegions(int num);`. So I'm saying still don't use `new`. – quamrana Feb 10 '10 at 12:14
  • @memC You can do it at construction time or later. You would have to provide a member function to do it of course. In either case, there is no need to create the objects with new. –  Feb 10 '10 at 12:15
1

Inside class A everything seems to be defined OK. (You will need to tidy up the class declaration). This means that everything will be automatically destroyed when an instance of A is destroyed. Yes, and I mean that std::vector reg; destroys itself, and all the copies of Region instances it has responsibility for.

Are you really asking about class Region and its vector which has pointers, which, although you haven't shown any code for might need its pointers deleting?

edit:

( first edit included details of Region, second edit removes them and adds A::addRegions )

I think this is what you are after:

#include <vector>

class Region{
   public:
      Region(int num);
      // Details of Region removed
};

class A{
    std::vector<Region> reg;
    const int numOfRegions;
public:
    A();
    // No destructor needed: ~A();
    void addRegions(int num);
};

A::A():numOfRegions(100){
    for (int i=0; i<numOfRegions; ++i){
        reg.push_back(Region(i));
      } 
}
void A::addRegions(int num)
{
    for (int i=0; i<num; ++i){
         reg.push_back(Region(i));
    } 
}

Notice how class A has no destructor.

new

A::addRegions also uses push_back and even now no destructor is needed.

Also notice how class declarations have semi-colons after them, and the constructor for A initialises numOfRegions.

quamrana
  • 37,849
  • 12
  • 53
  • 71
  • No, I am *not* asking about class Region and its vector. Your answer was all I needed... however, now I am confused whether I should use "new" or just keep the way the code it is. (confused because of @MSalters comment) Who should handle deletion of Region objects? Is useing "new" a better practice ? If so, should I also delete those in ~A()... or it will be taken care by vector itself? – memC Feb 10 '10 at 12:03
  • 1
    Best practise for what you have indicated is to let `std::vector` take care of managing the memory for instances of `Region`. – quamrana Feb 10 '10 at 12:04
  • yeap, I got it.. thanks! could you please respond to this question (added in a comment to Neeils reply) : "in this example, I illustrated that the Region instances are added within the constructor of class A. But, if I am to populate this list, at the run time, by calling a method on class A::addRegions(int num) , should I use "new" in this case? or should I just stick with my current code? " – memC Feb 10 '10 at 12:13
1

1. The only thing you'll need to explicitly delete here in your example are the elemt* if the they are dynamically allocated. If so, they must be deleted by however owns them. In the case your Region only points to them, you should not delete them in its destructor.

2. First, Your vector is allocated in the stack and will be deallocated without problems. Second, to you use new Region(i) you would need to change your class A declaration. But there is not a necessity for that in your case. A few examples when you should use operator new:
1) the necessity for a more persistent data is required, since your dynamically allocated data will persist until explicitily deallocated (and acessible as long you keep a pointer refering to it)
2) your objects might be too big to be all stored in the stack.

3. If you did use new Region(i), you would to use operator delete at some point, probably on destructor or other cleanup function, to correctly deallocate this.

4. Your constructor would present a problem only if you are creating new elemt directly in your elements vector i.e. elements.push_back(new elemt()), for instance. This is a dangerous design that may cause several memory leaks or risks for seg faults. If you want to allocate them this way, you would need a smart pointer with support for copy mechanics like a shared pointer.

fogo
  • 294
  • 1
  • 7