2
\$\begingroup\$

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");
        }
    }
};
\$\endgroup\$
2
  • \$\begingroup\$ It appears that some of your code has lost its indentation. \$\endgroup\$ Dec 24, 2014 at 14:35
  • \$\begingroup\$ @Matthew Read I have re-pasted the code. Does it look fine now? \$\endgroup\$ Dec 24, 2014 at 15:21

1 Answer 1

3
\$\begingroup\$

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
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks. a little unrelated, but is there something wrong with the indentation of my code? \$\endgroup\$ Dec 24, 2014 at 16:09
  • \$\begingroup\$ @morbidCode: Indentation looks fine. \$\endgroup\$ Dec 24, 2014 at 16:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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