0

Suppose that T contains an array whose size may vary depending on initialization. I'm passing a pointer to the vector to avoid copying all the data, and initialize as follows:

for(int i=10; i < 100; i++)
    std::vector.push_back(new T(i));

On exiting, one deletes the element's of the vector. Is there a risk of memory loss if the data contained in T is also a pointer, even if there are good destructors? Eg

template<class M> class T{
    M * Array;
public:
    T(int i) : Array(new M[i]){ }
    ~T(){ delete Array;}
};
Renan Gemignani
  • 2,683
  • 1
  • 21
  • 23
Plamen
  • 650
  • 1
  • 8
  • 27
  • From what I know it's bad style to put elements created with new in vectors. The vector's memory will be on the heap anyways ... – guitarflow Jan 23 '14 at 15:04
  • 1
    Why not use a vector inside `T`, to save the hassle of dealing with the [Rule of Three](http://stackoverflow.com/questions/4172722) yourself? And why not use `vector`, to save the hassle of deleting the elements when you remove them? – Mike Seymour Jan 23 '14 at 15:07
  • 1
    So, `T` has an underlying array of type `M`, and you're storing pointers to `T` in a `std::vector`? Why not just use `std::vector>` and avoid all the unnecessary manual memory management? – Chad Jan 23 '14 at 15:07
  • 2
    Plus, if you can `emplace_back` items you'll avoid copying stuff as well. – thokra Jan 23 '14 at 15:08
  • I want to use a pointer in the vector to avoid copying large data. – Plamen Jan 23 '14 at 15:11
  • @guitarflow unless created externally or by some API and the vector is used to keep track of the elements. (In which case one would probably want to use `std::shared_ptr` or `std::unique_ptr`, assuming the elements are actually C++ objects with destructors and aren't just pointers to dynamically-allocated C structs.) That doesn't seem to be the case here, but I had exactly that situation to deal with when working with an OpenGL wrapper provided to me for a graphics class that had been ported from C to C++ and thus still used many C-style concepts, including struct-pointer arguments everywhere. – JAB Jan 23 '14 at 15:11
  • @Plame: In C++11 you don't have to copy *anything* if you construct it in-place or *move* the element into the vector. If you're stuck with C++03 than you can employ the various answers already given. That being said, deletion still has to be handled properly by the respective type. – thokra Jan 23 '14 at 15:12
  • 1
    What is it exactly that you are trying to do? And don't say store a vector of pointers to a class that contains a pointer to an array. That's a terrible solution to any problem, and not the problem you want to solve here and now. – rubenvb Jan 23 '14 at 15:13
  • @JAB C-style concepts is exactly the situation I'm in! The 'M*' is C-style object. – Plamen Jan 23 '14 at 15:13
  • @Plamen: So use a vector, and call `data()` when you need a pointer to the underlying array. – Mike Seymour Jan 23 '14 at 15:27
  • 1
    @rubenvb: Judging from the code, the OP's aim is to simply have a dynamic array of dynamic arrays storing instances of `M`, i.e. `std::vector>` but is, probably due to existing code, stuck with `T` allocating a C-array of `M`s. – thokra Jan 23 '14 at 15:27

2 Answers2

3

There are two major problems with your class T:

  • You use delete rather than delete [] to delete the array, giving undefined behaviour
  • You don't implement (or delete) the copy constructor and copy-assignment operator (per the Rule of Three), so there's a danger of two objects both trying to delete the same array.

Both of these can be solved easily by using std::vector rather than writing your own version of it.

Finally, unless you have a good reason (such as polymorphism) to store pointers, use std::vector<T> so that you don't need to manually delete the elements. It's easy to forget to do this when removing an element or leaving the vector's scope, especially when an exception is thrown. (If you do need pointers, consider unique_ptr to delete the objects automatically).

Community
  • 1
  • 1
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
2

The answer is: don't.

Either use

std::vector<std::vector<M>> v;
v.emplace_back(std::vector<M>(42)); // vector of 42 elements

or (yuck)

std::vector<std::unique_ptr<M[]>> v;
// C++11
std::unique_ptr<M[]> temp = new M[42]; // array of 42 elements
v.emplace_back(temp);
// C++14 or with handrolled make_unique
v.emplace_back(std::make_unique<M[]>(42);

which both do everything for you with minimal overhead (especially the last one).

Note that calling emplace_back with a new argument is not quite as exception-safe as you would want, even when the resulting element will be a smart pointer. To make it so, you need to use std::make_unique, which is in C++14. Various implementations exist, and it needs nothing special. It was just omitted from C++11, and will be added to C++14.

rubenvb
  • 74,642
  • 33
  • 187
  • 332
  • Is the not-quite-as-exception-safe issue due to the possibility of an exception happening between construction of the M array and construction of its unique_ptr wrapper? – JAB Jan 23 '14 at 15:30
  • 1
    @JAB Yes, it happens here when the vector throws when it resizes to accomodate a new element, and leaves your allocated memory to suffer and die a lonely death. See comments [here](http://stackoverflow.com/a/3283795/256138) for a discussion and [here](http://stackoverflow.com/q/19472550/256138) for more examples of when it can bite you. – rubenvb Jan 23 '14 at 15:45
  • @rubenvb So, what is the difference between `std::make_unique(new M[42])` and `std::unique_ptr(new M[42])`? `make_unique` will be safer only when it not gets allocated pointer, but calls `new` itself. Also, when it's just one parameter there is no difference between constructing temporary `unique_ptr` and using `make_unique`, using latter will only make code more consistent. – Konstantin Oznobihin Jan 23 '14 at 16:15
  • @KonstantinOznobihin You are right on the calling of `make_unique` (I fixed the call), but not on the point of its usefulness. See [here](http://stackoverflow.com/questions/13172888/is-it-safe-to-use-emplace-back-with-a-container-of-unique-ptrs) for the question pretaining to exactly this case. – rubenvb Jan 23 '14 at 18:21
  • @rubenvb not at all, in the mentioned case there is `emplace_back(new T)` which is quite different from `emplace_back(unique_ptr(new T))`. – Konstantin Oznobihin Jan 24 '14 at 06:48