CODEWITHSUNDEEP
c++
Loay
Loay

Reputation: 601

unsafe template array constructor

This is probably a simple question but I have this template class:

template<typename Type>
class Array {
    size_t n;
    Type* buff;
public:
    Array(size_t n_): n(n_), buff(new Type[n]) {}
};

The code is from a course pdf file where it says buff(new Type[n]) is unsafe. I don't understand why it's unsafe, isn't size_t generally unsigned? Can I have an example where it could have a compile and/or run-time error?

Upvotes: 10

Views: 349

Answers (5)

Surt
Surt

Reputation: 16109

C++98 Standard 12.6.2.5.4 (I don't expect new versions to have relaxed this).

— Then, nonstatic data members shall be initialized in the order they were declared in the class definition (again regardless of the order of the mem-initializers).

So the order of initialization is defined according to this.

If you want an example of how to crash it simply make sizeof(Type)*n > total memory in your system.

Upvotes: 0

rustyx
rustyx

Reputation: 85481

First of all, you have a memory leak. But the question probably isn't about that. So let's assume you have a destructor that deallocates the array.

template<typename Type>
class Array {
    size_t n;
    Type* buff;
public:
    Array(size_t n_): n(n_), buff(new Type[n]) {}
    ~Array() { delete[] buff; }
};

Now this particular code is perfectly safe. No exception can be thrown while assigning n_, the order of initialization is correct and buff is the only raw pointer in your class. However, as you start expanding your class and writing more classes, the risk of a memory leak increases.

Imagine that you need to add one more members to the class Array:

template<typename Type>
class Array {
    size_t n;
    Type* buff;
    SomethingElse xyz;
public:
    Array(size_t n_): n(n_), buff(new Type[n_]), xyz(n_) {}
    ~Array() { delete[] buff; }
};

If the constructor of SomethingElse throws, the memory allocated for buff will leak, because the destructor ~Array() will never be called.

Modern C++ calls pointers such as Type* buff raw pointers because you are responsible for deallocating storage yourself (taking exceptions into account), and introduces tools such as std::unique_ptr and std::shared_ptr that can take care of storage deallocation automatically.

In modern C++ you could write your class like this:

template<typename Type>
class Array {
    size_t n;
    std::unique_ptr<Type[]> buff;
public:
    Array(size_t n_): n(n_), buff(new Type[n_]) {}
};

Notice the absence of a destructor. The unique_ptr will take care of calling delete for you.

Note also no dependency on class members inside the initializer list (simply writing new Type[n_] instead of new Type[n] makes your code more robust)

Upvotes: 3

cdonat
cdonat

Reputation: 2822

First of all the order, the initializations for the constructor are executed is not determined by the order they are written down, but by the order the initialized fields appear in the code:

class Array {
    size_t n;
    Type* buff;
public:
    Array(size_t n_): n(n_), buff(new Type[n]) {}
};

Here first n will be initialized and then buff.

class Array {
    Type* buff;
    size_t n;
public:
    Array(size_t n_): n(n_), buff(new Type[n]) {}
};

Now first buff will be initialized and then n, so n has no defined value in that case.

Using initialization lists for constructors is good practice, but be careful, that you don't create any assumptions on the order.

Generally it is a good idea to refrain from owning raw pointers. If you use smart pointers instead, you can not forget to release the data.

In the specific case, you might also want to use std::vector instead of a C-style array. That handles all the allocation, reallocation, releases, etc. in a thread safe manner for you. It seems like you are trying to write something like your own std::vector. Please only do that for educational purposes. Always prefer the standard implementation in production code. You probably won't get it better for quite a while. If you did, you would ask different questions here ;-)

Upvotes: 3

NathanOliver
NathanOliver

Reputation: 180965

The code is "unsafe" in the fact that it relies on n being constructed before buff. This dependency adds brittleness to the code.

When you construct the members of the class they are constructed in the order the are declared in the class, not how they are called in the member initialization list, so if the code was changed to

template<typename Type>
class Array {
    Type* buff;
    size_t n;
public:
    Array(size_t n_): n(n_), buff(new Type[n]) {}
};

Then when you do buff(new Type[n]), n is uninitialized and you have undefined behavior.

Upvotes: 12

feldi
feldi

Reputation: 151

It is not safe to call new operator inside initialization list. if new fails the destructor of Array will not be called. here is a similar question. Are there any issues with allocating memory within constructor initialization lists?

Upvotes: -1

Related Questions