Accelerated C++ 14-5: Custom string class and reference counter works for one constructor but not another

Question

For those of you familiar with the book Accelerated C++, I was writing a solution to problem 14-5 and came across some interesting behavior that I can't explain.

The problem involves using custom string and pointer/reference counter classes to implement a program that can concatenate vectors of strings and create pictures out of them.

Essentially, the part of the program in question is the following:

int main()
{
    vec<str> test;
    str s;

    while(getline(std::cin,s))
    {
            test.push_back(str(s.begin(),s.end()));
            //test.push_back(s); // This line doesn't work here - why?

            // Using the above line results in every str in test being 
            // the empty string
    }

    // Use the vec<str> to make pictures

}

It seems as though my reference counter isn't working properly when I use the commented line: the result I get is as if every str in test were the empty string.

Here are my implementations of getline and the relevant parts of the str and ptr classes:

str class:

class str
{
    friend std::istream& getline(std::istream &is, str &s);

public:
    typedef char* iterator;
    typedef const char* const_iterator;
    typedef size_t size_type;

    str() : data(new vec<char>) { }
    str(size_type n, char c) : data(new vec<char>(n,c)) { }

    str(const char *cp) : data(new vec<char>)
    {
            std::copy(cp,cp+std::strlen(cp),std::back_inserter(*data));
    }

    template <class InputIterator>
    str(InputIterator b, InputIterator e) : data(new vec<char>)
    {
            std::copy(b,e,std::back_inserter(*data));
    }

    // Other str member functions and operators
private:
    ptr< vec<char> > data;
};

ptr class:

template <class T>
class ptr
{
public:
    void make_unique()
    {
            if(*refptr != 1)
            {
                    --*refptr;
                    refptr = new std::size_t(1);
                    p = p ? clone(p) : 0;
            }
    }

    ptr() : p(0), refptr(new std::size_t(1)) { }
    ptr(T* t) : p(t), refptr(new std::size_t(1)) { }

    ptr(const ptr &h) : p(h.p), refptr(h.refptr) { ++*refptr; }
    ptr& operator=(const ptr &);
    ~ptr();

    T& operator*() const
    {
            if(p)
            {
                    return *p;
            }

            throw std::runtime_error("unbound ptr");
    }

    T* operator->() const
    {
            if(p)
            {
                    return p;
            }

            throw std::runtime_error("unbound ptr");
    }

private:
    T* p;
    std::size_t* refptr;
};

template <class T>
ptr<T>& ptr<T>::operator=(const ptr &rhs)
{
    ++*rhs.refptr;
    // free the left hand side, destroying pointers if appropriate
    if(--*refptr == 0)
    {
            delete refptr;
            delete p;
    }

    // copy in values from the right-hand side
    refptr = rhs.refptr;
    p = rhs.p;
    return *this;
}

template <class T>
ptr<T>::~ptr()
{
    if(--*refptr == 0)
    {
            delete refptr;
            delete p;
    }
}

The vec class is essentially a subset of std::vector. I can supply those details here too, if necessary.

And here is getline:

std::istream& getline(std::istream &is, str &s)
{
    s.data->clear();
    char c;
    while(is.get(c))
    {
        if(c != '\n')
        {
            s.data->push_back(c);
        }
        else
        {
            break;
        }
    }

return is;
}

Answer 1

Even though you are counting references correctly, you are still sharing the same pointer between the instances. So getline is modifying the same str object. You need to implement Copy-on-write in str.

Here is what's wrong:

std::istream& getline(std::istream &is, str &s)
{
    s.data->clear();          //should make a copy of data first
    char c;
    while(is.get(c))
    {
        if(c != '\n')
        {
            s.data->push_back(c);
        }
        else
        {
            break;
        }
    }

return is;
}

So, you should do:

s.data = ptr(new vec<char>());

instead of clearing the shared instance.

Answer 2

When you call:

test.push_back(s); // This line doesn't work here - why?

now s and a copy of s in test share the same data. During the next iteration of the while loop getline function calls s.data->clear(), which clears data in both s and the copy of s in test.

When you call:

test.push_back(str(s.begin(),s.end()));

the str(s.begin(),s.end()) constructor creates a temp str object with a copy of the data that was in s and pushes that object into test. So now the temp object and the copy in test share the same data, which is a non-shared copy of s. The temp object gets destroyed and the copy in test stays intact.