Pointer handle - absolute follow-up

Question

This is a follow-up to:

• Pointer handle - follow-up
• Pointer class/handle

Please review my pointer class.

template<typename T>
class Ptr {
public:
 Ptr(T* t, int s = 1) : sz{s<1 ? throw std::logic_error("Invalid sz parameter") : s} {
 sz = s;
 p = new T[sz];
 std::copy(t,t+sz,p);
 }
 Ptr(const Ptr& t) : Ptr(t.p, t.sz) { }
 Ptr& operator=(Ptr copy) {
 std::swap(copy.sz, sz);
 std::swap(copy.p, p);
 return *this;
 }
 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(index);
 return p[index];
 }
 T& operator[](int i) {
 check_range(i);
 return p[i];
 }
 T* get() const {
 return p;
 }
 void operator+=(int i) {
 check_range(index+i);
 index += i;
 }
 void operator-=(int i) {
 operator+=(-i);
 }
 Ptr operator+(int i) {
 Ptr old{*this};
 old += index+i;
 return old;
 }
 Ptr operator-(int i) {
 return operator+(-i);
 }
 Ptr& operator++() {
 operator+=(1);
 return *this;
 }
 Ptr operator++(int) {
 Ptr old{p+index};
 operator++();
 return old;
 }
 Ptr& operator--() {
 operator-=(1);
 return *this;
 }
 Ptr operator--(int) {
 Ptr<T> old{p+index};
 operator--();
 return old;
 }
 ~Ptr() {
 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");
 }
 if (p+i == nullptr) {
 throw std::out_of_range("null pointer");
 }
 }
};

• Loki Astari: Why did you change the void operator+=() to return void rather than a reference. Its not wrong just different from your previous questions.
• lightning_missile: @Loki Astari: I temporarily changed them to void while trying to find ways to not move/change p. It's already like that here codereview.stackexchange.com/questions/74918/…. Turned out they're unrelated I forgot to set them back to returning a reference. There's already an answer here when I found out it's still unchanged :)

Answer 1

Ptr(T* t, int s = 1): sz{s<1 ? throw std::logic_error("Invalid sz parameter") : s}
{
 sz = s; //this is unnecessary
 p = new T[sz];
 std::copy(t,t+sz,p);
}

Here you are writing to sz twice, first in the initializer list then in the constructors body. In general it's best if you just write only once in the initializer list if possible:

Ptr(T* t, int s = 1):
 sz{s<1 ? throw std::logic_error("Invalid sz parameter") : s}
{
 p = new T[sz];
 std::copy(t,t+sz,p);
}

In the move constructor:

Ptr(Ptr &&t):
 p{t.p},
 sz{t.sz}
{
 t.p = nullptr;
 t.sz = 0;
}

we are changing the pointer and size in this class but not the index. I would "move" everything and take the index from the other class here as well. We need to set t.p to nullptr so when t gets destructed the the call to delete[] p; will be a no-op, so that part is necessary. However I don't see why the sz needs to be set to 0 because it's a primitive and therefore not in the destructor. Because the range checking function always checks p for a nullptr we can't index the old object anyway so I think I would just remove that expression.

Ptr(Ptr &&t):
 p{t.p},
 sz{t.sz},
 index{t.index}
{
 t.p = nullptr;
}

(Disclaimer: I'm not especially experienced with c++ move semantics so if you think it's more readable to keep explicitly setting every element to a "null" type of value for readability please make a comment, I'd like to know what people think about that.)

• AndersK: personally I find it good to explicitly set sz to 0 from the point of readability -- but that is just my 2c.
• lightning_missile: Setting int to 0 is only for readability :)
• lightning_missile: Um, why would we also take index in the move constructor? For example if I am going to return a Ptr from a function, isn't it better if the index of the moved Ptr is at the very beginning of the array?
• shuttle87: @morbidCode, Generally speaking a move constructor moves everything in, so if your move constructor does not do that then it's probably a good spot to add some documentation. Explicitly setting the index back to 0 in the constructor would more clearly state your intention. If the index is not used in that way it would suggest to me that you could remove the index from the class entirely. What exactly is your intention with the index member?
• lightning_missile: In the previous versions of my code, I originally used it to move p whenever functions like operator++ and operator-- executes. Since changing p caused some problems during destruction, now I'm using index to hold p's current index whenever those functions got called without changing p. That's why operator*() returns p[index]. I created it because I can't change sz, I use it for check_range(i) and constructor. Is there a better way of doing this?