Why is creating STL containers dynamically considered bad practice?

Question

The name says it.

Bad practice example:

std::vector<Point>* FindPoints() { std::vector<Point>* result = new std::vector<Point>(); //... return result; } 

What is wrong if I delete this vector later?

I mainly program in C #, so this problem is not very clear to me in the context of C ++.

Answer 1

As a rule, you do not do this because the less you allocate a heap, the less you risk a memory leak. :)

std::vector is also useful because it automatically manages the memory used for a vector in RAII mode; by allocating it on the heap, you now need to explicitly free it (with delete result ) to avoid its memory leak. Things get complicated due to exceptions that can change your return path and skip any delete that you put in the path. (In C # you have no such problems because inaccessible memory is simply called periodically by the garbage collector)

If you want to return the STL container, you have several options:

• just return it by value; in theory, you should take over copying due to temporary ones created in the process of returning the result , but new compilers should be able to exclude the copy using NRVO ¹ . There may also be std::vector implementations that implement write-to-write optimization, as many std::string implementations do, but I never heard of that.

  In C ++ 0x compilers, you should run the move semantics instead, avoiding any copy.

• Store the result pointer in a smart pointer that transfers ownership, for example std::auto_ptr (or std::unique_ptr in C ++ 0x), and also change the return type to std::auto_ptr<std::vector<Point > > ; thus, your pointer is always encapsulated on a stack object, which is automatically destroyed when the function exits (in some way) and destroys vector if it still belongs to it. In addition, he fully understands who owns the returned object.
• Make the result vector a parameter passed by reference to the caller, and fill it, instead of returning a new vector.
• Hardcore STL option: instead, you will provide your data as iterators; then the client code will use std::copy + std::back_inserter or whatever to store such data in any container that it wants. Not much to see (it may be difficult to enter the code on the right), but worth mentioning.

---

• As @Steve Jessop noted in the comments, NRVO only works fully if the return value is used directly to initialize the variable in the calling method; otherwise, it can still evade the construction of a temporary return value, but the assignment operator to which the return value is assigned can still be called (see @Steve Jessop's comments for details).

Answer 2

Creating something dynamically is bad practice if it is really necessary. There is rarely a good reason to create a container dynamically, so this is usually not a good idea.

Edit: Usually, instead of worrying about things like quickly or slowly returning a container, most of the code should only deal with an iterator (or two) in the container.

Answer 3

Creating objects dynamically is generally considered bad practice in C ++. What if an exception is thrown from your "// ..." code? You can never delete an object. It is simpler and safer:

 std::vector<Point> FindPoints() { std::vector<Point> result; //... return result; } 

Shorter, safer, more severe ... In terms of performance, modern compilers will optimize the copy when they return, and if they cannot, the constructors will be moved, so this is still a cheap operation.

Answer 4

Perhaps you mean this recent question: C ++: vector <string> * args = new vector <string> (); calls SIGABRT

One insert: this is bad practice because it is a pattern that is prone to memory leak.

You force the caller to accept dynamic allocation and take responsibility for his service life. This is ambiguous from the declaration of whether the pointer returned by the pointer is a static buffer, a buffer belonging to another API (or object), or a buffer that now belongs to the caller. You should avoid this pattern in any language (including plain C) unless it clears the function name, which happens (e.g. strdup, malloc).

The usual way is to do this:

 void FindPoints(std::vector<Point>* ret) { std::vector<Point> result; //... ret->swap(result); } void caller() { //... std::vector<Point> foo; FindPoints(&foo); // foo deletes itself } 

All objects are on the stack, and all deletion is done by the compiler. Or just go back by value if you use the C ++ 0x + STL compiler or not against copying.

Answer 5

I like Jerry Coffin's answer. Also, if you want to avoid returning a copy, consider passing the result container as a reference, and sometimes you might need the swap () method.

 void FindPoints(std::vector<Point> &points) { std::vector<Point> result; //... result.swap(points); } 

Answer 6

Programming is the art of finding good compromises. Dynamically allocated memory can have a certain place, and I can even think of problems where a good compromise between code complexity and efficiency is achieved with std::vector<std::vector<T>*> .

However, std::vector does just fine with most of the needs of dynamically allocated arrays, and managed pointers are many times just the perfect solution for dynamically allocated single instances. This means that this is not so common when detecting cases where an unmanaged dynamically distributed container (or actually dynamically distributed) is the best compromise in C ++.

This, in my opinion, does not make the dynamic distribution “bad”, but simply “suspicious” if you see it in the code, because there is a high probability that better solutions may be possible.

In your case, for example, I see no reason to use dynamic allocation; just a function returning std :: vector would be efficient and safe. With any decent compiler, Return Value Optimization will be used when assigning to a just declared vector, and if you need to assign a result to an existing vector, you can still do something like:

 FindPoints().swap(myvector); 

which will not do any copying of the data, but simply hide the pointer (note that you cannot use the obviously more natural myvector.swap(FindPoints()) due to the C ++ rule, which is sometimes annoying, which prevents the transfer of temporary data as non-constant links).

In my experience, the biggest source of the needs of dynamically allocated objects are complex data structures in which the same instance can be reached using several access paths (for example, instances are both in a doubly linked list and indexed on a map), The standard library containers are always the only owner of the contained objects (C ++ - semantic language copies), so it may be difficult to implement these decisions effectively without indicator concept and din nomic allocation.

Often you can make enough reasonable tradeoffs that just use standard containers (maybe paying extra O (log N) requests that you could avoid), and that, given the much simpler code, IMO might be the best tradeoff in most cases .