2

I am working with writing the big five(copy constructor, copy assignment operator, move constructor, move assignment operator, destructor). And I've hit a bit of a snag with the copy constructor syntax.

Say I have a class foo that has the following private members:

    template<class data> // edit
    class foo{

    private:
        int size, cursor; // Size is my array size, and cursor is the index I am currently pointing at
        data * dataArray; // edit
    }

If I were to write a constructor for this of some arbitrary size X it would look like this.

template<class data> // edit
foo<data>::foo(int X){
    size = X;
    dataArray = new data[size];
    cursor = 0; // points to the first value
}

Now if I wanted to make a copy constructor of another object called bar I'd need to make the following:

template<class data> // edit
foo<data>::foo(foo &bar){
foo = bar; // is this correct? 
}

Assuming I have the overloaded = from the code below:

 template<class data> // edit
    foo<data>::operator=(foo &someObject){
        if(this != someObject){
            size = someObject.size;
            cursor = someObject.cursor;
            delete[] dataArray;
            dataArray = new data[size];
            for(cursor = 0; cursor<size-1;cursor++)
                 dataArray[cursor] = someObject.dataArray[cursor];
            }

        else
            // does nothing because it is assigned to itself
        return *this;
        }

Is my copy constructor correct? Or should foo = bar instead be *this = bar ?

I'm still new to templated constructors so if I made any errors in the code please let me know I will correct it.

EDIT 1: Thanks to the answer provided below by Marcin I have made some edits to the code above to make it more syntatically correct and commented them with //edit they are summarized in the list below:

  1. previously template<classname data>, which is incorrect must be template <typename data> or template <class data> for functions and classes respectively.
  2. previously int*dataArray; this missuses the template and should be data* dataArray;
Callat
  • 2,928
  • 5
  • 30
  • 47
  • 4
    **1.** Write the copy constructor to initialise each member from the r.h.s. object (much like your current `op=` does). **2.** Write your copy assignment in terms of the copy constructor. See the [copy and swap idiom](https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Copy-and-swap) (also [here](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom)) for details. The reason you should do it this way round is to avoid default-initialising then assigning when writing the copy constructor, which your current implementation does. – Andrew Jun 15 '16 at 11:30
  • 1
    Also see: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom – NathanOliver Jun 15 '16 at 11:36
  • 3
    It's `*this = bar`, and the copy constructor must take the source by `const` reference. Also, assignment operator is easier to implement and less error prone using the [copy and swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom?rq=1). But in this case, you should just replace `dataArray` and `size` with an `std::vector` and leave all the defaults for copy/move constructor and destructor; assignment operator also becomes trivial. – Matteo Italia Jun 15 '16 at 11:36
  • Be sure to use a [constructor member initialisation list](http://en.cppreference.com/w/cpp/language/initializer_list) (see also [here](http://stackoverflow.com/questions/926752/why-should-i-prefer-to-use-member-initialization-list)) when writing the constructor, otherwise you'll still be default constructing then assigning. – Andrew Jun 15 '16 at 11:37
  • 2
    Oh and by the way, mark your constructor from `int` as `explicit`. – Matteo Italia Jun 15 '16 at 11:38
  • @Andrew I'm still fairly new to programming so the idea of the copy-swap idiom is kind of difficult for me to wrap my head around. Can you post a solution so that I can view it as code and ask further questions there? And thank you for your support. – Callat Jun 15 '16 at 11:57
  • @NathanOliver You posted the exact same link that Andrew linked to above. – Callat Jun 15 '16 at 11:58
  • 1
    Oops. I didn't see he had a second link. I just saw the wiki link. – NathanOliver Jun 15 '16 at 11:59
  • @MatteoItalia So it MUST be `*this = bar`. This may sound dumb but I was under the impression that `explicit` should only be used when a constructor is a initializing a single parameter to avoid things like `someObject = 34;` where it may try an implicit type conversion. Am I wrong with my understanding of `explicit`? – Callat Jun 15 '16 at 12:01
  • 1
    @Hikari: yes, `explicit` is exactly for that reason; with your code as it is possible to e.g. mistakingly pass an integer to a function expecting a `foo`. It's not strictly related to the problem at hand (copy constructors), but it's worth noting. – Matteo Italia Jun 15 '16 at 12:18
  • 1
    Since the data members are not initialized: `delete[] dataArray;` will be a disaster. –  Jun 15 '16 at 12:20
  • 1
    @Hikari consider it done! I'd still suggest that in the example above you use a class like `std::vector` that takes care of the special member functions and ownership semantics for you. – Andrew Jun 15 '16 at 12:47
  • @DieterLücking OH! Because I am trying to `delete` a block of memory that has some garbage or arbitrary value because it's uninitialized right? – Callat Jun 15 '16 at 13:51

2 Answers2

2

The best way to achieve what you want is to use a class that already handles assignment, copying and moving, taking care of its memory management for you. std::vector does exactly this, and can directly replace your dynamically allocated array and size. Classes that do this are often referred to as RAII classes.


Having said that, and assuming this is an exercise in correctly implementing the various special member functions, I'd suggest that you proceed via the copy and swap idiom. (See What is the copy and swap idiom? on SO, for more details and commentary). The idea is to define the assignment operation in terms of the copy constructor.

Start with the members, constructor and destructor. These define the ownership semantics of the members of your class:

template <class data>
class foo {
 public:
  foo(const size_t n);
  ~foo();

 private:
  size_t size; // array size
  size_t cursor; // current index
  data* dataArray; // dynamically allocated array
};

template <class data>
foo<data>::foo(const size_t n)
 : size(n), cursor(0), dataArray(new data[n])
{}

template <class data>
foo<data>::~foo() {
    delete[] dataArray;
}

Here, memory is allocated in the constructor and deallocated in the destructor. Next, write the copy constructor.

template <class data>
foo<data>::foo(const foo<data>& other)
 : size(other.size), cursor(other.cursor), dataArray(new data[other.size]) {
     std::copy(other.dataArray, other.dataArray + size, dataArray);
}

(along with the declaration, foo(const foo& other); inside the class body). Notice how this uses member initialiser lists to set the member variables to the values in the other object. A new allocation is performed, and then in the body of the copy constructor you copy the data from the other object into this object.

Next comes the assignment operator. Your existing implementation has to perform a lot of manipulation of pointers, and isn't exception safe. Let's look at how this could be done more simply and more safely:

template <class data>
foo<data>& foo<data>::operator=(const foo<data>& rhs) {
  foo tmp(rhs); // Invoke copy constructor to create temporary foo

  // Swap our contents with the contents of the temporary foo:
  using std::swap;
  swap(size, tmp.size);
  swap(cursor, tmp.cursor);
  swap(dataArray, tmp.dataArray);

  return *this;
}

(along with the declaration in-class, foo& operator=(const foo& rhs);).

[-- Aside: You can avoid writing the first line (explicitly copying the object) by accepting the function argument by value. It's the same thing, and might be more efficient in some cases:

template <class data>
foo<data>& foo<data>::operator=(foo<data> rhs) // Note pass by value!
{
  // Swap our contents with the contents of the temporary foo:
  using std::swap;
  swap(size, rhs.size);
  swap(cursor, rhs.cursor);
  swap(dataArray, rhs.dataArray);

  return *this;
}

However, doing so may cause ambiguous overloads if you also define a move assignment operator. --]

The first thing this does is create a copy of the object being assigned from. This makes use of the copy constructor, so the details of how an object is copied need only be implemented once, in the copy constructor.

Once the copy has been made, we swap our internals with the internals of the copy. At the end of the function body, the tmp copy goes out of scope, and its destructor cleans up the memory. But this isn't the memory that was allocated at the beginning of the function; it's the memory our object used to hold, before we swapped our state with the temporary.

In this way, the details of allocating, copying and deallocating are kept where they belong, in the constructors and the destructor. The assignment operator simply copies and swaps.

This has a further advantage, over and above being simpler: It's exception safe. In the code above, an allocation error could cause an exception to be thrown while creating the temporary. But we haven't modified the state of our class yet, so our state remains consistent (and correct) even when the assignment fails.


Following the same logic, the move operations become trivial. The move constructor must be defined to simply take ownership of the resource and leave the source (the moved-from object) in a well-defined state. That means setting the source's dataArray member to nullptr so that a subsequent delete[] in its destructor doesn't cause problems.

The move assignment operator can be implemented similarly to the copy assignment, although in this case there's less concern with exception safety since you're just stealing the already-allocated memory of the source object. In the complete example code, I opted to simply swap the state.

A complete, compilable-and-runnable example can be seen here.

Community
  • 1
  • 1
Andrew
  • 5,212
  • 1
  • 22
  • 40
  • Thank you for the fantastic answer. I still don't quite get RAII yet but I think with practice on smaller test cases I will be able to get the hang of it. I really learned alot from your answer but in terms of simplicity I found the one below easier to implement for the problem I was having. So I accepted that one. – Callat Jun 16 '16 at 01:39
1

Your foo class does not internally use data template parameter. I suppose you wanted to use it here:

int * dataArray; // should be: data * dataArray;

You also are not allowed to use classname keyword but typename or class. You have also lots of other compile errors in your code.

Your copy constructor is wrong, it will not compile:

foo = bar; // is this correct? - answer is NO

foo is a class name in this context, so your assumption is correct. *this = someObject this would work (with additional fixes, at least dataArray must be set to nullptr), but your class variables would be default constructed first by copy constructor only to be overwritten by assignment operator, so its quiet non efficent. For more read here:

Calling assignment operator in copy constructor

Is it bad form to call the default assignment operator from the copy constructor?

Community
  • 1
  • 1
marcinj
  • 48,511
  • 9
  • 79
  • 100
  • Set to `nullptr`? Does this mean that my array should be initialized in the constructor? And does your answer mean that the `operator =(){}` is set up ok? I know this may be a bit troublesome but can you show me the other compile errors? I modified my code based off of a couple of things you pointed out. Please give it a look. – Callat Jun 15 '16 at 13:55
  • 1
    @Hikari if you do not set dataArray to nullptr before *this=bar; then operator= will call delete[] on dataArray with unspecified value. To see other places with compile errors compare your code with this: http://coliru.stacked-crooked.com/a/6f78eedbee39cf6a – marcinj Jun 15 '16 at 13:59