current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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");
        }
    }
};
share|improve this question
    
It appears that some of your code has lost its indentation. –  Matthew Read Dec 24 '14 at 14:35
    
@Matthew Read I have re-pasted the code. Does it look fine now? –  morbidCode Dec 24 '14 at 15:21

1 Answer 1

up vote 3 down vote accepted

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
    }
}
share|improve this answer
    
Thanks. a little unrelated, but is there something wrong with the indentation of my code? –  morbidCode Dec 24 '14 at 16:09
    
@morbidCode: Indentation looks fine. –  Loki Astari Dec 24 '14 at 16:10

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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