have I wrong order in assignment operator and cpy-ctor

[Tony Johansson]

This class template and main works perfectly fine. But could be better.
I have this class template called Handle that has a pointer declared as T*
body;
As you can see I have a reference counter in the class template so I know
how many references I have to the body. In my case it's the Integer wrapper
class which is the body.
This template works for many different types.
The one that I use is named Integer and is a Wrapper for an int. The code
for this is not include
because it doesn't add any futher information.
At the bottom I have a main.
In main I can call the stand alone function addNodeToSTL plus many more to
add nodes to the STL list container named myList1.
Here is the stand-alone function that add nodes to the STL list container.
void addNodeToSTL(list<handle_t>* myList)
{
handle_t myh(new Integer(getValue("Ange vardet som du vill lagga in i
listan/listorna: ")));
myList->push_front(myh);
}

This handle_t is typedef to
typedef Handle<Integer> handle_t;

Now to my question.
In this class template Handle I have one assignment operator and one copy
constructor.
This code for assignement operator and cpy-ctor. I thing the order is wrong
I must have statement
ref_count = h.ref_count; after
(*h.ref_count)++;
Now it's before so that's wrong order.
It's the same order mistake in both assignment operator and the cpy-ctor.
Do you agree with me.?
Below can you find the complete class template Handle and some parts of
main.

Handle(const Handle& h) //copy constructor
{
body = h.body;
ref_count = h.ref_count;
(*ref_count)++;
}

const Handle& operator=(const Handle& h) //assignment operator
{
if (this != &h)
{
(*ref_count)--;
if (!*ref_count)
deleteAll();
ref_count = h.ref_count;
body = h.body;
(*h.ref_count)++;
}
return *this;
}

//Tony


template<class T>
class Handle
{
public:
Handle()
{
body = new T(0);
ref_count = new int(1);
}

Handle(T* body_ptr) //Constructor
{
body = body_ptr;
cout << "Body adressen = " << body << endl;
ref_count = new int(1);
}

~Handle() //Destructor
{
(*ref_count)--;
if (!*ref_count)
deleteAll();
}

T operator*() const { return *body; }

T& operator*()
{
if (*ref_count > 1)
{
(*ref_count)--;
body = new T(*body);
ref_count = new int(1);
}
return *body;
}

T* operator->()
{
if (*ref_count > 1)
{
(*ref_count)--;
body = new T(*body);
ref_count = new int(1);
}
return body;
}

T* operator->() const { return body; }
bool operator==(const T& h) { return *body == h; }
bool operator==(const Handle& h) { return *body == *h.body; }

Handle(const Handle& h)
{
body = h.body;
ref_count = h.ref_count;
(*ref_count)++;
}

int getRefCount() { return *ref_count; }
void operator()(Handle& h) { cout << *h.body << endl; }

const Handle& operator=(const Handle& h)
{
if (this != &h)
{
(*ref_count)--;
if (!*ref_count)
deleteAll();
ref_count = h.ref_count;
body = h.body;
(*h.ref_count)++;
}
return *this;
}

private:
T* body;
int* ref_count;

void deleteAll()
{
delete body;
body = NULL;
delete ref_count;
ref_count = NULL;
}
};

void addNodeToSTL(list<handle_t>* myList)
{
handle_t myh(new Integer(getValue("Ange vardet som du vill lagga in i
listan/listorna: ")));
myList->push_front(myh);
}
typedef Handle<Integer> handle_t;

int main()
{
list<handle_t>* myList1 = new list<handle_t>;
list<handle_t>* myList2 = new list<handle_t>;
do
{
switch (huvudMeny())

{
case 0: return 0;
case 1: addNodeToSTL(myList1);
break;
case 2: displaySTL(myList1, myList2);
break;
case 3: deleteSelectedValue(myList1);
break;
case 4: deleteAll(myList1);
break;
case 5: findSelectedValue(myList1);
break;
case 6: testOperatorStar(myList1);
break;
case 7: testOperatorArrow(myList1);
break;
case 8: copyList(myList1, myList2);
break;
}
} while(1);
return 0;
}

//Many thanks

//Tony

 

[Ivan Vecerina]

: This class template and main works perfectly fine. But could be
better.
....
: Now to my question.
: In this class template Handle I have one assignment operator and one
copy
: constructor.
: This code for assignement operator and cpy-ctor. I thing the order
is wrong
: I must have statement
: ref_count = h.ref_count; after
: (*h.ref_count)++;
: Now it's before so that's wrong order.
: It's the same order mistake in both assignment operator and the
cpy-ctor.
: Do you agree with me.?
I do not think that this is a problem in this case. The
only thing to be aware of is that, in either case, the
behavior will be undefined in a multi-threaded program
(an atomic operation or a lock would be needed then).

: Below can you find the complete class template Handle and some parts
of
: main.
:
: Handle(const Handle& h) //copy constructor
: {
: body = h.body;
: ref_count = h.ref_count;
: (*ref_count)++;
: }
This is ok.

: const Handle& operator=(const Handle& h) //assignment operator
: {
: if (this != &h)
: {
: (*ref_count)--;
: if (!*ref_count)
: deleteAll();
: ref_count = h.ref_count;
: body = h.body;
: (*h.ref_count)++;
: }
: return *this;
: }
Ok as well. Note that the self-assignment test
becomes unnecessary if you order the operations
properly. The whole function can be written as:
++*h.ref_count;
if( 0 == --*ref_count ) deleteAll();
body = h.body;
ref_count = h.ref_count;

: //Tony
:
:
: template<class T>
: class Handle
: {
: public:
: Handle()
: {
: body = new T(0);
Note that whis will fail if T cannot be initialized
with 0 - making Handle less generic than needed.
You could write instead:
body = new T( T() ); // explicit call to dflt-ctr
// will initialize int and any
// built-in type to zero.

: ref_count = new int(1);
: }
:
: Handle(T* body_ptr) //Constructor
It is safer to make this constructor explicit:
explicit Handle(T* body_ptr)
(this will help avoid unexpected conversions).

: {
NB: it would probably be good practice to test
that body_ptr is not null (assert or throw), as
the rest of the code assumes/implies it never is.

: body = body_ptr;
: cout << "Body adressen = " << body << endl;
: ref_count = new int(1);
: }
:
: ~Handle() //Destructor
: {
: (*ref_count)--;
: if (!*ref_count)
: deleteAll();
: }
:
: T operator*() const { return *body; }
Usually the following function signature would
be used instead:
const T& operator*() const { return *body; }
If T is a complex class (as expected if a Handle
class is needed), this may avoid unnecessary
object copies.

: T& operator*()
: {
: if (*ref_count > 1)
: {
: (*ref_count)--;
: body = new T(*body);
: ref_count = new int(1);
: }
: return *body;
: }
Ok, so this implements copy-on-write (COW).
This has many caveats for the user, but
this function looks ok.

: T* operator->()
: {
: if (*ref_count > 1)
: {
: (*ref_count)--;
: body = new T(*body);
: ref_count = new int(1);
: }
: return body;
: }
:
: T* operator->() const { return body; }
The returned type should be const for consistency:
T const* operator() const { return body; }

: bool operator==(const T& h) { return *body == h; }
: bool operator==(const Handle& h) { return *body == *h.body; }
NB: these are best implemented as 'friend' operators. E.g.:
friend bool operator==(const Handle& a, const Handle& b)
{ return *a.body = *b.body; }
This ensures that x==y and y==x are always equivalent,
in particular if implicit conversions are involved.

: Handle(const Handle& h)
: {
: body = h.body;
: ref_count = h.ref_count;
: (*ref_count)++;
: }
:
: int getRefCount() { return *ref_count; }
This probably can be: ^const

: void operator()(Handle& h) { cout << *h.body << endl; }
Hum... unusual semantics.

: const Handle& operator=(const Handle& h)
: {
: if (this != &h)
: {
: (*ref_count)--;
: if (!*ref_count)
: deleteAll();
: ref_count = h.ref_count;
: body = h.body;
: (*h.ref_count)++;
: }
: return *this;
: }
:
: private:
: T* body;
: int* ref_count;
:
: void deleteAll()
: {
: delete body;
: body = NULL;
: delete ref_count;
: ref_count = NULL;
: }
: };
:
: void addNodeToSTL(list<handle_t>* myList)
NB: it would be more appropriate to use a reference:
void addNodeToSTL(list<handle_t>& myList)

: {
: handle_t myh(new Integer(getValue("Ange vardet som du vill lagga
in i
: listan/listorna: ")));
: myList->push_front(myh);
: }
: typedef Handle<Integer> handle_t;
:
: int main()
: {
: list<handle_t>* myList1 = new list<handle_t>;
: list<handle_t>* myList2 = new list<handle_t>;
: do
: {
: switch (huvudMeny())
:
: {
: case 0: return 0;
: case 1: addNodeToSTL(myList1);
: break;
: case 2: displaySTL(myList1, myList2);
: break;
: case 3: deleteSelectedValue(myList1);
: break;
: case 4: deleteAll(myList1);
: break;
: case 5: findSelectedValue(myList1);
: break;
: case 6: testOperatorStar(myList1);
: break;
: case 7: testOperatorArrow(myList1);
: break;
: case 8: copyList(myList1, myList2);
: break;
: }
: } while(1);
: return 0;
: }
:
: //Many thanks
:
: //Tony

I hope this helps,
Ivan