6
\$\begingroup\$

This is a pointer class of mine that I would like to have reviewed. Right now it only allocates in the stack and has no copy or move operations.

template<typename T>
class Ptr {
public:
    Ptr(T* t, int s = 1) :p{t},sz{s} { }
    T& operator*() {
        check_range();
        return *p;
    }
    T& operator[](int i) {
        check_range(i);
        return p[i];
    }
    Ptr& operator+(int i) {
        index += i;
        check_range(index);
        p+= i;
        return *this;
    }
    Ptr& operator-(int i) {
        index -= i;
        check_range(index);
        p -= i;
        return *this;
    }
    Ptr& operator++() {
        *this = *this+1;
        return *this;
    }
    Ptr operator++(int) {
        Ptr<T> old{p};
        ++(*this);
        return old;
    }
    Ptr& operator--() {
        *this = *this-1;
        return *this;
    }
    Ptr operator--(int) {
        Ptr<T> old{p};
        --(*this);
        return old;
    }
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");
        }
    }
};

How can I make this shorter and less ugly? Is there a better solution than what I did?

\$\endgroup\$
7
\$\begingroup\$

I would expect operator+ and operator- to return a new object (leaving the current one intact.

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

If I was using normal pointers.

 T* x  = getAT();
 x + 1;           // Does not change x
 T* y = x + 1;    // Does not change x (but y has the value of x+1).

Usually you see operator+ defined in terms of operator+=

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

The *this = *this+1; look very strange.

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

I would have just called the operator+=

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

Again don't like ++(*this);

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

I would have used:

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

You should strive for the strong exception guarantee (also known as transaction exception guarantee).

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

Though this code looks like it does that. It is usually called in a way that prevents it.

    index -= i;
    check_range(index);  // If this throws your object is now in the new state
                         // with index out of range. I would have tried to make sure
                         // that if an exception is thrown then the state of the object
                         // remains unchanged.


    // Change to this:

    check_range(index - i); // If it throws then the object is still
                            // in a consistent unchanged state.
    index -= 1;             // If we get here then change the state.
\$\endgroup\$

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.