CODEWITHSUNDEEP
c++operatorsoperator-overloading
Rafael S. Calsaverini
Rafael S. Calsaverini

Reputation: 14022

Overloading = operator in C++ when there are arrays of values to be copied

I'm somewhat new to C++ so, I guess this is a very basic question.

Suppose I have this class:

// file Graph.h
class Graph { 
public:
  Graph(int N); // contructor
  ~Graph();     // destructor  
  Graph& operator=(Graph other);
private:
  int * M;
  int N;
};

// file Graph.cpp
Graph :: Graph(int size) {
  M = new int [size];
  N = size;
}

Graph :: ~Graph() {
  delete [] M;
}

I want to create an assignment operator that will copy the contents of array M[] but not to overwrite it when I change it after the copy (I think this is accomplished by not copying the actual pointer but only the content, don't know if I'm right). This is what I've tried:

Graph& Graph::operator=(Graph other) {
  int i;
  N = other.N;
  M = new int [N];
  for (i = 0; i < N; i++)
    M[i] = other.M[i];
  return *this;
 }

Is this correct? Are there other ways to do this?

edit: An important question I forgot. Why I must declare it like Graph& operator=(Graph other); and not just: Graph operator=(Graph other); which is what's written in my book (C++: The Complete Reference, 2nd ed, Herbert Schildt, pages 355-357)?

Upvotes: 1

Views: 2056

Answers (8)

davka
davka

Reputation: 14692

almost all have been said, but there are a few important notes:

  • it is advised to pass the parameter to copy constructor and assignment operator as const reference, i.e. const Graph& other, to avoid heavy copying to a temp object (unless you are using a "copy and swap" idiom
  • if you use pointer as a class member you must (well, should in most cases) provide both copy ctor and assignment operator, or disable them by making them private, otherwise the default ones will cause a leak.
  • finally, why not use std::vector? will save you all this trouble without noticeable performance penalty.

this page may be helpful - simple and comprehensive: C++ Operator Overloading Guidelines

Upvotes: 1

Salizer
Salizer

Reputation: 11

Some of it have already been said by others, but if we want to stick with basic layout that you have, you should change this:

  • Make "Graph other" constant -> "const Graph other" (Good to do, so you don´t 'accidentally' change the data in "other")
  • Have a check on "other" to see that it is not the same object as the object you are assigning to(the Lvalue). Can do this by making a simple if statement -> if(this == &other)
  • Delete the memory in M. (We don want memory leaks :) )

Oh btw. You don´t have to use C syntax by declaring "i" in the start of the function -> "for(int i =...."

Hope it helped :)

Upvotes: 1

Alexandre C.
Alexandre C.

Reputation: 56976

The canonical way is to use a std::vector<int> to avoid managing memory yourself. For the exercise though, the right way to do what you want is:

#include <algorithm>

class Graph
{
public:    
    Graph(size_t n) { data_ = new int[n](); size_ = n; }

    Graph(Graph const& g)
    {
        data_ = new int(g.size_);
        size_ = g.size_;
        std::copy(g.data_, g.data_ + g.size_, data_);
    }

    ~Graph() { delete[] data_; }

    void swap(Graph& g) throw()
    {
        std::swap(data_, g.data_);
        std::swap(size_, g.size_);
    }

    Graph& operator=(Graph g) { g.swap(*this); return *this; }

private:
    int* data_;
    size_t size_;
};

Google "copy and swap idiom" for the rationale behind the code. Note that your assigment operator leaks memory (the original array is overwritten but never deleted) and if the allocation fails, you end up with a broken object. Moreover, x = x won't do what it is expected to do. These three pitfalls are common, and writing assignment operators in copy-and-swap style avoids them.

EDIT: For your other question, returning a reference allows you to chain assignments, like a = b = c, which is valid for built in types. It may or may not be what you want (it usually is).

Upvotes: 9

Francesco
Francesco

Reputation: 3250

You must return a reference to the object that you have constructed. If you returned a copy, you would then proceed to copy construct another object, discarding the one that you already have.

Upvotes: 0

Lou Franco
Lou Franco

Reputation: 89222

You need the & in operator= so that it doesn't cause a copy when you return *this. The more important question is why do you need to return anything. The answer is to support a=b=c syntax.

I would suggest you use memcpy for standard arrays of built-in int-like types (or pointers). The standard defines it in a way that gives the compiler writer a way to provide the fastest possible, platform-specific implementation.

Do not use memcpy if the type is an object (won't call copy constructor and a lot of other bad things)

Upvotes: 3

Chowlett
Chowlett

Reputation: 46677

Don't forget that it's legal to write graph_a = graph_a; your code will leak the memory originally allocated for graph_a.M, and you'll end up with uninitialised memory in M after the copy.

Before doing a copy-assignment, you must check that you're not copying the same object over itself (in which case you can just return).

Upvotes: 0

lijie
lijie

Reputation: 4871

You'd probably want to declare and define a copy constructor.

In fact, it is a must in your situation because the default copy constructor will result in a double delete during destruction.

I think it's more idiomatic to use the copy constructor in the assignment operator (a copy construction followed by a swap).

As for the current code, there is a memory leak (because the old M is not deleted).

Upvotes: 1

Marek Szanyi
Marek Szanyi

Reputation: 2328

You could also use std::copy more here std::copy

or you can memcpy arrays as well

Upvotes: 0

Related Questions