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\$
  • \$\begingroup\$ It appears that some of your code has lost its indentation. \$\endgroup\$ – Matthew Read Dec 24 '14 at 14:35
  • \$\begingroup\$ @Matthew Read I have re-pasted the code. Does it look fine now? \$\endgroup\$ – morbidCode Dec 24 '14 at 15:21
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\$
  • \$\begingroup\$ Thanks. a little unrelated, but is there something wrong with the indentation of my code? \$\endgroup\$ – morbidCode Dec 24 '14 at 16:09
  • \$\begingroup\$ @morbidCode: Indentation looks fine. \$\endgroup\$ – Martin York Dec 24 '14 at 16:10

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.