5

I want to have a copy of the currently running instance.

When i change a value in the copy, original object is also affected. The copy acts as an instance.

How to avoid this? I need to create an independent copy of the calling object.

 Set operator+(Set s){
             Set temp = *this;  

             for(int i=0; s.elements[i] != '\0'; i++){
                     temp(s.elements[i]);
             }
             temp.elements[0] = 'X'; // <- this affects calling object also :(

             return temp;

         }
coder9
  • 1,571
  • 1
  • 27
  • 52
  • What is this syntax? i mean temp(s.elements[i])?? it is not a function! – Amir Zadeh Oct 20 '10 at 08:07
  • Use the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). – GManNickG Oct 20 '10 at 08:54
  • @GMan: That's something to implement assignment. What's this got to do with `operator+()`? – sbi Oct 20 '10 at 10:29
  • @sbi: the idea could also be applied if you have operator+= working: `Set Set::operator+(Set const& rhs) { Set temp = *this; operator+=(rhs); swap(temp); return temp; }` . It's of course a lot easier to just write `{ Set temp = *this; temp += rhs; return temp; }` – MSalters Oct 20 '10 at 10:58
  • 1
    @MSalters: Indeed, a lot easier! (Plus I'd call this the canonical implementation.) – sbi Oct 20 '10 at 11:42
  • 1
    @Sara @MSalters (Old, I know): `operator+` should be a free-function. Generally, of the form: `Set operator+(Set lhs, const Set& rhs) { lhs += rhs; return lhs; }`; free-function should be used over member-functions, it allows a copy to be elided when you add to a temporary, and has the best performance in C++0x, where you guarantee moves both into and from the function. – GManNickG Oct 30 '10 at 22:31

6 Answers6

8

The problem is that Set temp = *this; makes a shallow copy, not a deep copy. You will have to modify the copy constructor and assignment operators for the Set class so that they make copies of all the member/contained objects.

E.g:

class Set
{
public:
    Set()
    {
        elements = new SomeOtherObject[12];
        // Could make elements a std::vector<SomeOtherObject>, instead
    }

    Set(const Set& other)
    {
        AssignFrom(other);
    }

    Set& operator=(const Set& other)
    {
        AssignFrom(other);
        return *this;
    }

private:
    void AssignFrom(const Set& other)
    {
        // Make copies of entire array here, as deep as you need to.
        // You could simply do a top-level deep copy, if you control all the
        // other objects, and make them do top-level deep copies, as well
    }

    SomeOtherObject* elements;
};
Merlyn Morgan-Graham
  • 58,163
  • 16
  • 128
  • 183
  • I had already overloaded = operator as i needed to do unions in sets. Set operator=(Set s){ elements = s.elements; //overrides element of "this" return *this; } – coder9 Oct 20 '10 at 07:42
  • 1
    @Sara: There's a difference between __assignment__ (change an existing, initialized object) and __copy-construction__ (initialize an object for the first time). If you need one, you likely need both plus the destructor. See [The Rule of Three](http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29). – sbi Oct 20 '10 at 07:49
  • @Merlyn: Considering this is an answer for a newbie, your code should IMO not leave out the dtor. – sbi Oct 20 '10 at 11:44
  • Hi, Can you please have a look at the code i've added below? thanks in advance... – coder9 Oct 20 '10 at 13:26
  • 3
    Please don't implement your Big Three this way. Use the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). – GManNickG Oct 20 '10 at 16:45
  • @Gman, sbi: Good points. I don't program in C++ daily, and forget to use idioms like this (thankfully I make sure to bone up on them again when I actually write code for pay, but apparently I got lazy here =P). I will edit the answer to implement this correctly, and post links to big-three and copy-and-swap idioms. – Merlyn Morgan-Graham Oct 20 '10 at 18:26
5

Not that your function already makes two copies, since it takes its argument and returns its result per copy:

Set operator+(Set s);

So you wouldn't have to copy s, because it's already copied. I suppose this is involuntarily, so you might want to read about how to pass objects to functions and how to return objects from function in C++.

The problem you're reporting, though, hints at your copy constructor not working properly. Did you implement the copy constructor or are you using the compiler-supplied one?

Community
  • 1
  • 1
sbi
  • 219,715
  • 46
  • 258
  • 445
3

This probably depends on how Set is implemented. If the assignment operator and the copy constructor haven't been overloaded to do a deep copy(including elements) then it won't work as expected.

CodesInChaos
  • 106,488
  • 23
  • 218
  • 262
2

Have you implemented a copy constructor for your class? Default copy constructor will copy any pointer in your class, but not the content you are pointing to. You need to create a copy constructor or overload the '=' operator.

Benoit Thiery
  • 6,325
  • 4
  • 22
  • 28
1

I would avoid a char pointer completely and use std::string instead. This way you dont even need a copy constructor and an assigment operator because the compiler generated once will do just fine. (because 'elements' of the 'Set' class is copy-constructible and has an assignment operator) Here is my solution:

#include <iostream>
#include <string>

class Set{
  std::string elements;

  public:
         Set() {
             elements = "";
         }

         explicit Set(char* _elements) {
             if (_elements)
                elements = _elements;
         }

         Set operator+(const Set& s){
             Set temp(*this);    

             temp.elements += s.elements;
             return temp;
         }



};

Btw. I added a constructor from char* so that 'elements' can somehow be initialized from outside. Not sure if this is what you wanted.

Enni74
  • 69
  • 1
0

Ok. I went through rule of three and did the following changes... Can you point out what's wrong with this?

#include<iostream>
#include<cstring>

using namespace std;

class Set{
  char *elements;

  public:
         Set() {
              elements = new char('\0');
              index = -1;
         }

         Set(const Set& cpy){
                  *this = cpy;
         }


         Set operator+(Set s){
             Set temp = *this;       // IMPORTANT! copy constructor of Set is called, "this" is passed as argument
                                     // * = current OBJECT, else returns ADDRESS of current object

             for(int i=0; s.elements[i] != '\0'; i++){
                     temp(s.elements[i]);
             }

             return temp;

         }
         Set& operator=(Set s){  
              delete [] elements;
             elements = new char[strlen(s.elements) + 1];
             strcpy(elements,  s.elements); //overrides element of "this"

             return *this;
         }

    };
coder9
  • 1,571
  • 1
  • 27
  • 52
  • Your code looks like a better start, but you are missing a destructor. Check out RAII and the big three/rule of three. http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization, http://www.parashift.com/c++-faq-lite/coding-standards.html#faq-27.10 – Merlyn Morgan-Graham Oct 20 '10 at 18:58
  • For one, your code is not compiling. In your constructor Set(), you are using `index = -1`; where `index` is not declared anywhere. Secondly, in your operator overloading method `operation+(Set s)`, you are using `temp(s.elements[i])` where `temp` is not a function; it's an object! What are you trying to do? – Jaywalker Oct 28 '10 at 11:12