4
\$\begingroup\$

This is a followup to:

Please review this 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& t) {
        sz = t.sz;
        p = new T[sz];
        T* x = t.get();
        for (int i = 0; i < sz; ++i) {
            p[i] = x[i];
        }
    }

    Ptr& operator=(const Ptr& t) {
        Ptr copy{t};
        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();
        return *p;
    }

    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;
        p+= i;
    }

    void operator-=(int i) {
        check_range(index-i);
        index -= i;
        p -= i;
    }

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

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

    Ptr& operator++() {
        operator+=(1);
        return *this;
    }

    Ptr operator++(int) {
        Ptr<T> old{p};
        operator++();
        return old;
    }

    Ptr& operator--() {
        operator-=(1);
        return *this;
    }

    Ptr operator--(int) {
        Ptr<T> old{p};
        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");
        }
    }

    void check_range() {
        if (p == nullptr) {
            throw std::out_of_range("null pointer");
        }
    }

};
\$\endgroup\$
4
\$\begingroup\$

c++11 Allows constructor chaining:

Ptr(const Ptr& t) {
    sz = t.sz;
    p = new T[sz];
    T* x = t.get();
    for (int i = 0; i < sz; ++i) {
        p[i] = x[i];
    }
}

Can be written as:

Ptr(const Ptr& t): Ptr(t.p, t.sz) {}

Good to see you using the copy and swap idiom.

Ptr& operator=(const Ptr& t) {
    Ptr copy{t};
    std::swap(copy.sz, sz);
    std::swap(copy.p, p);
    return *this;
}

But his can be simlified by making the compiler to the copy.

Ptr& operator=(Ptr copy) {
        //   ^^^^^    Pass by value is generating a copy.       
    std::swap(copy.sz, sz);
    std::swap(copy.p, p);
    return *this;
}

This looks good.

~Ptr() {
    delete[] p;
}

But you ca still move p

void operator+=(int i) {
    check_range(index+i);
    index += i;
    p+= i;               // P has changed
}

void operator-=(int i) {
    check_range(index-i);
    index -= i;
    p -= i;               // P has changed
}

Thus using it with delete after the point it changed is going to cause UB.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I still don't understand the dangers on moving p. I've read that delete[] deletes the whole array right? So if I move p anywhere as long as it's within range I thought I can still delete the array. Can you explain further? When will it cause undefined behavior? \$\endgroup\$ – morbidCode Dec 27 '14 at 6:16
  • 2
    \$\begingroup\$ The value passed to delete must be the same value that was returned by a call to new. So if you change the value of p it is not the original value and thus not a valid input to delete. \$\endgroup\$ – Martin York Dec 27 '14 at 9:13
3
\$\begingroup\$

Operators

You have a bit of duplication with some of the operators:

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

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

Here we can rewrite the - operator in terms of the + operator as follows:

Ptr operator-(int i) {
    return operator+(-i);
}

We can do the same for -= and += too. The main benefit here is that we are reducing the amount of duplicated code.

Constructors

One thing I would do is to use the initializer list to initialize as much as possible in the constructors:

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

It would probably be a good idea to check that the size parameter is not negative. When we have a raw array like this we can't easily initialize the array in this manner in the initializer list. If we used a std::array as storage this would be a lot easier, though given the tag that would somewhat defeat the purpose of the question I suppose. This then poses a bit of a design question, you could pass the size of the array in as a template parameter then initialize the space for the array using that template parameter or you can do what you have here.

One other thing you might consider is to use std::copy to initialize the array:

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

Bounds checking

void check_range(int i) {
    if (i < 0 || i > sz-1) {
        throw std::out_of_range("out of range");
    }
}

This doesn't check if p is not null, will the operators that call this form of the range check always be presented with a valid pointer?

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Actually, I want to remove the array size as a template parameter. I really want my constructor to just get the size on its own so it'd be easier to use. I just haven't figured out how to do it with raw arrays :) \$\endgroup\$ – morbidCode Dec 27 '14 at 6:05
  • \$\begingroup\$ Regarding check_range(int i), I thought as long as the element is within range it couldn't be null. But maybe I should check it as well just to make sure. \$\endgroup\$ – morbidCode Dec 27 '14 at 9:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.