-2

I've nearly finished my smart pointer (I know...) so I uploaded it to my univerity's website which runs a number of automated tests on my code. There are two things wrong with a number of tests:

  • Memory or time limit is exceeded
  • Memory access problem (ie. null pointer)

The thing is I don't know what kind of tests are being excecuted. I am able to read the automated tests' stdout which I did, this is written there:

In instantiation of ‘my_pointer<T>::my_pointer() [with T = tester]’:
error: no matching function for call to ‘tester::tester()’
note: candidates are:
tester::tester(my_pointer<tester>)
candidate expects 1 argument, 0 provided
tester::tester(const tester&)
candidate expects 1 argument, 0 provided

So I'm guessing for some odd reason it won't invoke my my_pointer() constructor? This is my smart pointer class:

template<class T>
class my_pointer {
    T* raw_pointer;

public:
    my_pointer() {
        raw_pointer = new T();
        raw_pointer->incRefCnt();
    }

    my_pointer(T *obj) : raw_pointer(obj) {
        if(raw_pointer != NULL) raw_pointer->incRefCnt();
    }

    my_pointer(const my_pointer<T>& smart_pointer) : raw_pointer(smart_pointer.raw_pointer) {
        if(raw_pointer != NULL) raw_pointer->incRefCnt();
    }

    T& operator*() {
        return *raw_pointer;
    }

    T* operator->() {
        return raw_pointer;
    }

    operator T*() {
        return raw_pointer;
    }

    my_pointer<T> &operator=(const my_pointer<T> &smart_pointer) {
        if(this != &smart_pointer && raw_pointer != NULL) {
            /** if this was the last reference to the given memory address */
            if (raw_pointer->decRefCnt() == 0) {
            delete raw_pointer;                    
            }

            raw_pointer = smart_pointer.raw_pointer;
            raw_pointer->incRefCnt();
        }

        return *this;
    }

    bool operator== (const T* pointer) {
        return raw_pointer == pointer;
    }

    bool operator!= (const T* pointer) {
        return raw_pointer != pointer;
    }

    bool operator== (const my_pointer<T> &smart_pointer) {
        return raw_pointer == smart_pointer.raw_pointer;
    }

    bool operator!= (const my_pointer<T> &smart_pointer) {
        return raw_pointer != smart_pointer.raw_pointer;
    }

    ~my_pointer() {
       if(raw_pointer->decRefCnt() == 0 && raw_pointer != NULL) {
           delete raw_pointer;
        }
    }
};

This is a class, in which the references can be counted:

class refcounted {
private:
    int count;
public:
    refcounted() : count(0) { }

    int incRefCnt() {
        return ++count;
    }

    int decRefCnt() {
        return --count;
    }
};

Can you see any problems with the code? Thanks in advance!

masm64
  • 1,222
  • 3
  • 14
  • 31
  • 3
    That's a curious design. Your smart pointer class can only be used with types that inherit from `refcounted`? – Christian Hackl Nov 01 '15 at 16:00
  • 4
    Wouldn't you want to decrement the ref count in your destructor and delete if it's 0? – Kenney Nov 01 '15 at 16:01
  • Also note that the `if(raw_pointer != NULL) ` is redundant. So is `raw_pointer = NULL;` in the destructor. – Christian Hackl Nov 01 '15 at 16:03
  • Did you do any tests before submitting your design? Even a most rudimentary test would catch this problem. – n. m. could be an AI Nov 01 '15 at 16:05
  • 1
    @ChristianHackl, actually I've seen such design in [`QSharedDataPointer`](http://doc.qt.io/qt-4.8/qshareddatapointer.html) (scroll for an example a bit if follow the link) – Lol4t0 Nov 01 '15 at 16:06
  • @Lol4t0: Interesting. I don't think it's good design, anyway. I am not too familiar with Qt, but I guess most of its API is from an era in which most C++ code was too OOP-centric anyway. – Christian Hackl Nov 01 '15 at 16:06
  • Another hint: Your assignment operator is needlessly complicated. Use the Copy-and-Swap Idiom (http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). Even the `!= this` check would then not be necessary. – Christian Hackl Nov 01 '15 at 16:08
  • @ChristianHackl Qt has a lot of _that_ things. I believe it is from era without variadic templates when implementing `make_shared` was a challenging task – Lol4t0 Nov 01 '15 at 16:08
  • @Lol4t0: Yeah, but `make_shared` is just the icing on the top. Remember that we've had `boost::shared_ptr` long before C++11. – Christian Hackl Nov 01 '15 at 16:09
  • @ChristianHackl http://www.boost.org/doc/libs/master/libs/smart_ptr/intrusive_ptr.html – n. m. could be an AI Nov 01 '15 at 16:15
  • @ChristianHackl sadly yes, but this is not my design but my professor's – masm64 Nov 01 '15 at 16:19

2 Answers2

3

You are unconditionally deleting your raw pointer in the destructor. This is wrong. You must decrement the reference count, and only delete if it becomes zero.

You also call raw_pointer->incRefCnt(); in several places without checking whether raw_pointer is NULL.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
  • I've done as you said, now it is better. But it still gives me an error for some tests: "Memory access problem (ie. null pointer)" – masm64 Nov 01 '15 at 16:36
  • Oh and this is the stdout: chkpt #1 construct 0 chkpt #2 chkpt #3 memberselect 0 chkpt #4 chkpt #5 chkpt #6 – masm64 Nov 01 '15 at 16:41
  • Did I miss something? The destructor doesn't dec the reference count. – Robinson Nov 01 '15 at 16:42
  • Whoops sorry, I've just updated the code. I've ran some local test but those just used the constructors and the = operator. For some reason when I call the delete operator on an object which has the type my_pointer, it does nothing at all. I've no idea what kind of tests are being run on the server. – masm64 Nov 01 '15 at 16:51
  • Now you never check anything for NULL.. How do you expect your class handle NULL pointers? – n. m. could be an AI Nov 01 '15 at 17:17
  • I guess I should change the two if statements to include raw_pointer != NULL && raw_pointer->decRefCnt() == 0) { // etc } – masm64 Nov 01 '15 at 17:19
  • well, I guess everywhere where I use the pointer (in a meaning that I use pointed object's method) first I should see if the pointer is NULL or not. – masm64 Nov 01 '15 at 17:20
  • when my_pointer() : raw_pointer(0) { } is used, shouldn't you be incrementing ref count in raw_pointer constructor? – Nandu Nov 01 '15 at 17:25
  • yes, but I was thinking how to implement that properly. If I set raw_pointer to 0 then it means it is not referring to any object, right? Because then I can't call raw_pointer->incRefCnt() I guess – masm64 Nov 01 '15 at 17:28
  • Or maybe I can do something like this in C++: raw_pointer = new T(); – masm64 Nov 01 '15 at 17:34
  • Oh it is working, nice. You can't do this in Java so I though you can't do something like this in C++, too :) xd – masm64 Nov 01 '15 at 17:38
  • I've updated the code, and written down the new error message – masm64 Nov 01 '15 at 17:59
0

I see two problems in your code:

  1. In operator=(): you check if(raw_pointer != NULL) after you've already dereferenced raw_pointer (on the line above). You should reverse the checks.

  2. In the destructor: shouldn't you first call decRefCnt(), and only delete raw_pointer if the reference count has dropped to zero?

Since you need practically the same code in both operator=() and ~my_pointer() for decreasing the reference count and deleting raw_pointer if needed (AFAICS), it seems more practical to extract that functionality to a new method and call that from both places.

Roel Schroeven
  • 1,778
  • 11
  • 12