Problem
I have tried to implement a copy on write string class with value semantics and reference counting.
I wanted to support indexing, so I have overloaded the operator[] and to differentiate between the l-value and r-value usage it is returning a proxy object.
#include <iostream>
#include <string.h>
class proxy;
//Reference counting class.Also holds the string.
class RC
{
public:
RC(const char * str);
~RC();
private:
char* str;
int count;
friend class mystring;
friend class proxy;
};
class mystring
{
public:
mystring(const char* str);
mystring& operator=(const mystring& str);
mystring(const mystring& str);
~mystring();
void print() const;
proxy operator[](int n);
private:
RC *pRC;
friend class proxy;
};
class proxy
{
public:
proxy(mystring* r,int n);
proxy& operator=(const char& c);
proxy& operator=(const proxy& c);
operator char() const;
private:
int n;
mystring* ptr;
};
RC::RC(const char * str1)
:str(new char[strlen(str1)+1]),count(1)
{
strcpy(str,str1);
}
RC::~RC()
{
if(--count == 0)
delete [] str;
}
proxy mystring::operator[](int n)
{
return proxy(this,n);
}
mystring::mystring(const char* str)
:pRC(new RC(str))
{
}
mystring& mystring::operator=(const mystring& rhs)
{
if(this != &rhs)
{
if(--pRC->count == 0)
delete pRC;
pRC = rhs.pRC;
++pRC->count;
}
return *this;
}
mystring::mystring(const mystring& rhs)
{
pRC = rhs.pRC;
++pRC->count;
}
mystring::~mystring()
{
if(--pRC->count == 0)
delete pRC;
}
void mystring::print() const
{
std::cout << pRC->str << ":" << pRC->count << 'n';
}
proxy::operator char() const
{
return ptr->pRC->str[n];
}
proxy::proxy(mystring* r,int n)
:ptr(r),n(n)
{}
proxy& proxy::operator=(const char& c)
{
if(c != ptr->pRC->str[n])
{
char cpy[strlen(ptr->pRC->str)+1];
strcpy(cpy,ptr->pRC->str);
ptr->pRC->~RC();
cpy[n] = c;
ptr->pRC = new RC(cpy);
}
return *this;
}
proxy& proxy::operator=(const proxy& rhs)
{
if(this != &rhs)
{
*this = (char)rhs;
}
return *this;
}
Can you please help me improve the code?
Solution
Code simplification
mystring
acts essentially as a smart pointer. It’s already implemented in the standard library. You can use the std::shared_ptr
class. This way, mystring
can just have an std::shared_ptr<std::string>
as a member. The default copy constructor, assignment operator and the desturctor will work properly.
Use of non-standard features
char cpy[strlen(ptr->pRC->str)+1]
variable sized arrays are not a part of the standard C++. Don’t use them. If you need it, you should allocate it dynamically.
If you use the std::shared_ptr<std::string>
as I said above, it’ll be even simpler as std::string
has a proper copy constructor.
Index
It’s conventional to use size_t
, not int
as an index type (at least that’s how an std::string
works).
My suggestions:
Make RC
a private, nested class
RC
is an implementation detail of mystring
. It’s best to make it a private, nested class.
RC
does not follow the rule of three
RC
allocates memory in its constructor and deallocates the memory in the destructor. All such classes should follow The Rule of Three.
You can get away with disregarding the rule of three if RC
is a private, nested class. Even then, there is a possibility that you will run into problems if the implementations of any of the member functions of mystring
is not mindful of that fact.
Provide operator[]
function in RC
You had to make proxy
a friend
of RC
in order to be able to access strc
. Had you provided an operator[]
function, that would have been necessary. Given the choices, I would recommend providing operator[]
function(s) in RC
.
Fix the memory leak in RC
You have:
RC::~RC()
{
if(--count == 0)
delete [] str;
}
However, the destructor of RC
is called only when count
is equal to 0
in couple of places.
In mystring::operator=(const mystring& rhs)
:
if(--pRC->count == 0)
delete pRC;
In mystring::~mystring
:
if(--pRC->count == 0)
delete pRC;
That means, you need to change the destructor of RC
to be:
RC::~RC()
{
// No need for this. count is already 0
// if(--count == 0)
delete [] str;
}
Use a less cryptic name instead of RC
Examples:
mystring::ReferenceCountedData
mystring::Data
mystring:Implementation
I would go with mystring::Data
. The fact that it is reference counted is evident from the goal of mystring
.
Provide a const
version of the operator[]
function
The current version of operator[]
is a non-const
function. If a function has a const
object of type mystring
, it won’t be able to use the operator[]
function get access the contents of the object. You should provide a const
version of the operator[]
function.
char operator[](size_t index) const
{
return pRC->str[index];
}
If you would like to use a proxy
for that function as well, you will need to use another class called const_proxy
. That is analogous to the use of iterator
and const_iterator
in the container classes in the standard library.
const_proxy operator[](size_t index) const
{
return const_proxy(this, index);
}
I’ll leave the implementation of const_proxy
as a detail.
Avoid mystring::print
I would recommend using an overload of operator<<
. That makes mystring
usable with all kinds of ostream
objects. It also makes the task of outputting objects of the class more idiomatic.
std::ostream& operator<<(std::ostream& out, mystring const& s)
{
return (out << pRC->str << ":" << pRC->count);
}
Now, you can use:
mystring obj("My Test String");
std::cout << obj << std::endl;