Pointer class/handle

Question

Please review my pointer class.

template<typename T>
class Ptr {
public:
 Ptr(T* t, int s = 1) {
 sz = s;
 p = new T[sz];
 for (int i = 0; i < sz; ++i) {
 p[i] = t[i];
 }
 }
 Ptr(const Ptr&) = delete;
 Ptr& operator=(const Ptr&) = delete;
 Ptr(Ptr &&t) :p{t.p}, sz{t.sz} {
 t.p = nullptr;
 t.sz = 0;
 }
 Ptr& operator=(Ptr &&t) {
 std::swap(t.p,p);
 std::swap(t.sz,sz);
 return *this;
 }
 T& operator*() {
 check_range();
 return *p;
 }
 T& operator[](int i) {
 check_range(i);
 return p[i];
 }
 Ptr& operator+=(int i) {
 check_range(index+i);
 index += i;
 p+= i;
 return *this;
 }
 Ptr& operator-=(int i) {
 check_range(index-i);
 index -= i;
 p -= i;
 return *this;
 }
 Ptr& operator+(int i) {
 Ptr old{*this};
 return old+=1;
 }
 Ptr& operator-(int i) {
 Ptr old{*this};
 return old-=1;
 }
 Ptr& operator++() {
 return operator+=(1);
 }
 Ptr operator++(int) {
 Ptr<T> old{p};
 operator++();
 return old;
 }
 Ptr& operator--() {
 return operator-=(1);
 }
 Ptr operator--(int) {
 Ptr<T> old{p};
 operator--();
 return old;
 }
 ~Ptr() {
 while (index < sz-1) {
 operator++();
 }
 while (index != 0) {
 delete p;
 operator--();
 }
 delete p;
 }
private:
 T* p;
 int sz;
 int index = 0;
 void check_range(int i) {
 if (i < 0 || i > sz-1) {
 throw std::out_of_range("out of range");
 }
 }
 void check_range() {
 if (p == nullptr) {
 throw std::out_of_range("null pointer");
 }
 }
};

• Matthew Read: It appears that some of your code has lost its indentation.
• lightning_missile: @Matthew Read I have re-pasted the code. Does it look fine now?

Answer 1

Only need to point out one thing.

Constructor:

 p = new T[sz];

Destructor:

 delete p;

This code is broken. If you allocation with new [] you MUST destroy with delete [].

In the operator+= and operator-=

Ptr& operator+=(int i) {
 check_range(index+i);
 index += i;
 p+= i;
 return *this;
}
Ptr& operator-=(int i) {
 check_range(index-i);
 index -= i;
 p -= i;
 return *this;
}

Thus moving p during the lifetime of the object is not a good idea (because you must call delete on the pointer returned by new). So if you are going to move p you need to keep track of the original pointer in another member. But I would not move it all. You are adjusting index so why adjust p?

These can't return by reference:

Ptr& operator+(int i) {
 Ptr old{*this};
 return old+=1;
}
Ptr& operator-(int i) {
 Ptr old{*this};
 return old-=1;
}

You have to return by value because you are creating new values.

I am surprised that this works:

 Ptr old{*this};

Because you have deleted the copy constructor.

Ptr(const Ptr&) = delete;

If it compiles I am not sure what is happening. But it is definitely not good.

In the constructor you should prefer initializer list.

Ptr(T* t, int s = 1) {
 sz = s; // Initializer list
 p = new T[sz]; // Initializer list.
 // This seems inefficient.
 // Since the line above has just called the constructor on each element
 // element in the array. You are now calling the assignment operator
 // on each element in the array.
 for (int i = 0; i < sz; ++i) {
 p[i] = t[i];
 }
}

You can optimize the above

Ptr(T* t, int s = 1)
 : sz(s)
 , p(reinterpret_cast<T*>(new char[s * sizeof(T)]) // Allocate aligned but uninitialized memory.
{
 for (int i = 0; i < sz; ++i) {
 new (p + i) T(t[i]); // Its called placement new.
 // The `(p + i)` is the address where you want the
 // construction to take place (ie don't allocate)
 // Then we use the copy constructor to create the
 // copy
 }
}

• lightning_missile: Thanks. a little unrelated, but is there something wrong with the indentation of my code?
• Loki Astari: @morbidCode: Indentation looks fine.