1

I have a structure/class with a member that is a pointer, let's say

struct myStruc
{
    int* m_p;
}

1. Question: Where should I delete the pointer? In the destructor?

myStruct::~myStruct()
{
    delete m_p;
}

2. Question: What if the pointer is assigned to an pointer array, e.g.

myStruct mS;
mS.m_p = new int[3];

Is there a nice way (no dynamic_cast or try-catch) to now if I have to do delete or delete[]?

ezdazuzena
  • 6,120
  • 6
  • 41
  • 71
  • A/The way is to allocate the memory in the constructor and deallocate it in the destructor. – Kiril Kirov Apr 10 '13 at 14:32
  • Can you tell more about the use case? What is the purpose of myStruc? Who is setting m_p? What does m_p point to? Who decides and knows this? – Werner Henze Apr 10 '13 at 14:40
  • 1
    [Item 20: Avoid data members in the public interface.](http://www.amazon.com/Effective-Specific-Improve-Programs-Designs/dp/0321334876) – Dave Rager Apr 10 '13 at 14:54
  • 2
    @DaveRager While that is good general advice, a `struct` is usually just a collection of data with not much additional functionality and these data use to be `public` because there should not be much need for encapsulation. At least that is what I understand when I see that a class is defined as `struct`. – Gorpik Apr 10 '13 at 15:25

3 Answers3

3

You shouldn't ALLOW the "user" to mess with pointers within your data structures. Make the pointer private and use get/set functions to access it.

So:

struct myStruc
{
  private:
    int* m_p;
  public:
    int* ptr() { return m_p; }
    void allocate(int n) { m_p = new int[n]; 
    myStruc() : m_p(0) {};
    ~myStruc() { delete [] m_p; }
};

Edit: The above class is NOT COMPLETE, it is showing the concept. For a complete class you would need a copy constructor and an assignment operator, and probably "remember" n from the allocate function.

Now you never have to worry about wheter you allocaed with [] or not, since it's consistent.

Of course, you could achieve this with using std::vector instead, without any extra effort.

Mats Petersson
  • 126,704
  • 14
  • 140
  • 227
  • Your code has a problem. It does not follow the Rule of three. This is a disaster waiting to happen. – Alok Save Apr 10 '13 at 14:52
  • @AlokSave: What exactly are you referring to? The missing copy constructor? – ezdazuzena Apr 10 '13 at 15:03
  • 1
    @ezdazuzena: Yes. In this case whenever the object is copied the implicitly generated copy constructor will be used and it does not make a deep copy of your data member. This basically means you end up with two objects referring to the same dynamically allocated memory, once one of them goes out of scope it deallocates the memory and gives you a dangling pointer and UB. It can be argued that the class is not copyable or assignable but for that both the copy constructor and assignment operator should be declared as `private` and should not be provided with any definition. – Alok Save Apr 10 '13 at 15:06
  • Edit added to explain that it's "not complete". – Mats Petersson Apr 10 '13 at 15:22
2

Ideally, you should not use a raw pointer member at all. You have two better options:

  1. Use a std::vector or
  2. Use a smart pointer as member and choose the right one as per your usage semantic.

If you cannot use a smart pointer and must use a raw pointer then:

  • If you allocate the pointer using new then use delete, if you use new [] then you need to use delete []. There should be no mismatch.
  • You need to call delete or delete [] everytime the lifetime of this dynamically allocated member ends. Assuming the lifetime of this member is same as lifetime of your class it will be in the destructor.
  • The important part is that you should follow The Rule of Three.
Community
  • 1
  • 1
Alok Save
  • 202,538
  • 53
  • 430
  • 533
  • 1
    @ezdazuzena: You don't need boost. `std::vector` is part of language since C++03 while if you are using C++11 you have a variety of smart pointer from which you can choose one. – Alok Save Apr 10 '13 at 14:43
  • I agree with this answer. Saying that the pointer should not be `public` misses the point here if we assume that the class has been correctly defined as `struct`. – Gorpik Apr 10 '13 at 15:29
0
  1. Ideally in destructor. But follow the Rule of Three.
  2. You should use delete[]. new/new[] should use delete/delte[] respectively.
Community
  • 1
  • 1
Mahesh
  • 34,573
  • 20
  • 89
  • 115
  • With respect to 2.: How do I now that new int[3] was used? What if `mS.m_p = new int();` was performed? `delete[]` would fail then. – ezdazuzena Apr 10 '13 at 14:34
  • Why ideally in the destructor? How can the struct know if it should delete the pointer or not, or whether to call `delete` or `delete[]`? – juanchopanza Apr 10 '13 at 14:38
  • @juanchopanza From my perspective, raw pointer usually returns it's resources in destructor. I answered the delete part of your comment in 2. – Mahesh Apr 10 '13 at 14:41
  • 1
    There is no way for the destructor to know whether `new[]` or `new` has been used. – juanchopanza Apr 10 '13 at 14:43
  • @juanchopanza I am sorry if my statement was ambiguous. It's the programmer's responsibility to return the acquired resources. All I am saying in 2 is that if programmer has acquired resources using new/new[], then a corresponding delete/delete[] must be used. – Mahesh Apr 10 '13 at 14:49