views:

371

answers:

6

Here's what I've got:

void set::operator =(const set& source)
{
    if (&source == this)
        return;

    clear();

    set(source);
}

And here's the error I get:

vset.cxx:33: error: declaration of 'source' shadows a parameter

How do I properly do this?

A: 

That error is usually the result of having a local variable named the same as a function argument. Can you post more of your code?

Bill Zeller
+2  A: 

I believe with set(source); you are trying to call copy ctor. You can not do that in C++ i.e. you can not explicitly invoke the ctor. What you can do is write a private clone method and call it in both copy ctor and assignment operator.

Naveen
Damn, thats lame. Thanks!
Ring
@Ring: I would recommend that you read up on object lifetimes in C++. The fact that you can't call a constructor on something that is already constructed is very much part of the constructed once, destroyed once guarantee that you have in C++ for all objects (assuming no dirty tricks). It's one of the things that makes constructors in C++ more than just post-construction initialization functions that they are in many other languages. Definitely not lame (IMHO)!
Charles Bailey
Clone() is the Java way of handling things. It is not the standard way of doing things in C++
Martin York
A: 

The error message you are seeing doesn't match the question, but I don't know that it is relevant. The answer to your question is that you cannot call the copy constructor from inside your code because the object is already constructed.

If you want to avoid code duplication between the copy constructor and operator=, I suggest having a private function that does the common work, and call that from both the copy constructor and operator=.

For reference, you could call operator= from copy constructor by doing:

*this = source;

However, I don't think that is a good idea especially if you have virtual functions or if the operator=() function assumes a fully constructed object (which it probably does).

janm
+1  A: 

As you've noted, set(source); is the source (no pun intended) of the problem. This isn't doing quite what you think it is -- it's not attempting to invoke a copy ctor. Instead, it's basically equivalent to: set source; -- i.e. it's attempting to define a set object named source -- the parentheses are redundant but allowed.

You can invoke a copy ctor in a ctor (or just about anywhere you want to) but it's not going to to what you want anyway -- a copy ctor creates a copy, so even if you did invoke it, it would just create a temporary object, which would evaporate at the end of that statement.

As already mentioned, what you probably want is a private function to copy the data from one object to another, then use that from both your copy ctor and your copy assignment operator. Better still, define it using objects that can be handled correctly by the default copy ctor and copy assignment operators.

Jerry Coffin
+8  A: 

You are looking for the copy swap idiom:

set& set::operator=(set const& source)
{
    /* You actually don't need this. But if creating a copy is expensive then feel free */
    if (&source == this)
        return;

    /*
     * This line is invoking the copy constructor.
     * You are copying 'source' into a temporary object not the current one.
     * But the use of the swap() immediately after the copy makes it logically
     * equivalent.
     */
    set tmp(source);
    this->swap(tmp);

    return *this;
}

void swap(set& dst) throw ()
{
    // swap member of this with members of dst
}
Martin York
Advantage of copy-swap idiom is that it separates concerns: copy ctor may throw exception, which is OK at this point because you haven't changed any state in 'this' instance; once copy is made, swap is used that exchanges state of 'this' with the copy w/o risk of exception.
Pavel Repin
A: 

I don't know the details of what your header file specifies, but I would try something like this (you may need to modify it for your specific application):


void set::operator =(const set& source)
{

if (this == &source)
  {
     return;
  }

size_t i;
this->clear();
data=source.data;
for (i=0; i<source.child.size(); i++)
  {
     child.push_back(new set(*(source.child[i])));
  }

}


-Joel

Joel Dee
Looks like a good strategy.
Joel Dee