• Advertisement
Sign in to follow this  

C++ code is it ok

This topic is 4401 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Just a couple of quick queries. Is it ok to overload the assignment operator (=) and then use it in a copy constructor like this.
Task::Task(const Task& t){
  *this = t;
}

also, in the operator assignment why is it that scope isn't an issue. I'm using g++ (cygwin/Eclipse) and it doesn't seem to mind me doing this.
Task& Task::operator=(const Task& t){
  task_from = t.task_from;
  task_to = t.task_to;
}

When both task_from and task_to are private. Does it have something to do with it being the same class?

Share this post


Link to post
Share on other sites
Advertisement
For your first question- I generally write out assignment operators and copy constructors separately but I'm not sure if there are any drawbacks either way so I'll let someone else field that.

For your second question- yes, since it is the same class it is allowed to access it's own and other instances of the same class' private members from within its own member functions

Share this post


Link to post
Share on other sites
For the first, I'm almost positive that that is just fine. I don't remember if I ever used an overloaded = in constructor, but it should work.

For the second, since = is a member function, it can change those two private variables. And the way it knows which task_from or task_to is which, is that, a "this" dereference is assumed when not shown.

i.e. THIS:

task_from = t.task_from;
task_to = t.task_to;

is equivalent to THIS:

this->task_from = t.task_from;
this->task_to = t.task_to;

(or THIS too... I'm just playing with syntax now...):

(*this).task_from = t.task_from;
(*this).task_to = t.task_to;

Share this post


Link to post
Share on other sites
It is ok, but it is likely that if you need a custom copy constructor and assignment operator, you can't implement the copy constructor using the assignment operator. Generally, a custom assignment operator has to release some resource or deallocate some memory. Doing that might cause a problem in a copy constructor where there is no resource to release or memory to deallocate. Here is a simple example:
    class foo
{
foo()
{
p = new int;
}
// assignment operator
foo & operator=( foo & rhs )
{
if ( this != &rhs )
{
delete p;
p = new int( *rhs.p );
}
return *this;
}
// copy constructor
foo( foo const & rhs )
{
*this = rhs; // major problem: this->p is uninitialized so delete p will barf.
}
};

}

Share this post


Link to post
Share on other sites
It's okay, the STL has you covered:


#include <memory>
#include <iostream>

struct MyObject
{
MyObject()
{
std::cout << "new " << this << std::endl;
}

MyObject( MyObject const & ref )
{
std::cout << "new " << this << " (from " << &ref << ")" << std::endl;
}

~MyObject()
{
std::cout << "del " << this << std::endl;
}
};

struct foo
{
typedef std::auto_ptr<MyObject> MyObjectPointer;
MyObjectPointer p;

foo(): p( new MyObject )
{}

foo & operator=( foo const & rhs )
{
if ( this != &rhs )
p = MyObjectPointer(new MyObject( *rhs.p ));

return *this;
}

foo( foo const & rhs )
{
*this = rhs;
}
};

int main()
{
foo a, b, c = a;

a = b = c;
}






$ g++ -ggdb3 -Wall -Wextra -Werror -ansi -pedantic test.cpp -o test
$ valgrind ./test
        <snip>
new 0x1bb36028
new 0x1bb36060
new 0x1bb36098 (from 0x1bb36028)
new 0x1bb360d0 (from 0x1bb36098)
del 0x1bb36060
new 0x1bb36108 (from 0x1bb360d0)
del 0x1bb36028
del 0x1bb36098
del 0x1bb360d0
del 0x1bb36108
==21675==
==21675== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 21 from 1)
==21675== malloc/free: in use at exit: 0 bytes in 0 blocks.
==21675== malloc/free: 5 allocs, 5 frees, 5 bytes allocated.
==21675== For counts of detected errors, rerun with: -v
==21675== No malloc'd blocks -- no leaks are possible.

Share this post


Link to post
Share on other sites
Quote:
Original post by willow01
Just a couple of quick queries. Is it ok to overload the assignment operator (=) and then use it in a copy constructor like this.

*** Source Snippet Removed ***

also, in the operator assignment why is it that scope isn't an issue. I'm using g++ (cygwin/Eclipse) and it doesn't seem to mind me doing this.

*** Source Snippet Removed ***

When both task_from and task_to are private. Does it have something to do with it being the same class?
I may work in many cases, but is not a totally safe practice. The reason is that in an assignment operation, the left hand side has already been constructed, and is merely being overwritten / replaced.
However in this case the left hand argument being passed in has not yet been initialised.
This can make trouble in cases such as where the assignment operator first checks if some field in both objects are equal, before copying, for instance.

In many cases it will be fine. People can and do use it, without problems in many cases. However you have now been warned of the potential dangers of doing so.

Edit: John Bolton appears to have already demonstrated exactly this, and very nicely I might add - DOH![smile]

Share this post


Link to post
Share on other sites
Quote:
Original post by willow01
Is it ok to overload the assignment operator (=) and then use it in a copy constructor like this.


No. Just don't do it. There are many and varied, and sometimes subtle, reasons why it won't always work, or will work but not the way you expect. For a book-length treatment of the topic, try Sutter's Exceptional C++.

What you might want to do is implement your assignment operator in terms of the copy consructor. If you have a member function void swap() nothrow(), what you've done is provided the strong exception guarantee, and all your object-copying smarts is in a single function, the copy constructor (which will throw an exception if the class invariants are not met).

class C
{
public:
C(const C& rhs);

C& operator=(const C& rhs)
{
C tmp(rhs);
swap(tmp);
return *this;
}

protected:
void swap(const C& rhs) nothrow();
};

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement