c++: operator = is ambiguous when implementing move assignment

Question

I am trying to implement a rule of five for the first time. After reading a lot of recommendation about best practices, I end up with a solution where copy/move assignment operators seem to be in some conflict.

Here is my code.

#include <vector>   
#include <memory>

template<class T> class DirectedGraph {
public:
    std::vector<T> nodes;
    DirectedGraph() {}
    DirectedGraph(std::size_t n) : nodes(n, T()) {}
    // ... Additional methods ....
};

template<class T>
DirectedGraph<T> Clone(DirectedGraph<T> graph) {
    auto clone = DirectedGraph<T>();
    clone.nodes = graph.nodes;
    return clone;
}

template<class T> class UndirectedGraph
{
    using TDirectedG = DirectedGraph<T>;
    using TUndirectedG = UndirectedGraph<T>;

    std::size_t numberOfEdges;
    std::unique_ptr<TDirectedG> directedGraph;
public:
    UndirectedGraph(std::size_t n)
        : directedGraph(std::make_unique<TDirectedG>(n))
        , numberOfEdges(0) {}

    UndirectedGraph(TUndirectedG&& other) {
        this->numberOfEdges = other.numberOfEdges;
        this->directedGraph = std::move(other.directedGraph);
    }

    UndirectedGraph(const TUndirectedG& other) {
        this->numberOfEdges = other.numberOfEdges;
        this->directedGraph = std::make_unique<TDirectedG>
            (Clone<T>(*other.directedGraph));
    }

    friend void swap(TUndirectedG& first, TUndirectedG& second) {
        using std::swap;
        swap(first.numberOfEdges, second.numberOfEdges);
        swap(first.directedGraph, second.directedGraph);
    }

    TUndirectedG& operator=(TUndirectedG other) {
        swap(*this, other);
        return *this;
    }

    TUndirectedG& operator=(TUndirectedG&& other) {
        swap(*this, other);
        return *this;
    }

    ~UndirectedGraph() {}
};

int main()
{
    UndirectedGraph<int> graph(10);
    auto copyGraph = UndirectedGraph<int>(graph);
    auto newGraph = UndirectedGraph<int>(3);
    newGraph = graph;            // This works.
    newGraph = std::move(graph); // Error here!!!
    return 0;
}

Most of the recommendations I took from here, and I implemented copy assignment operator= to accept parameter by value. I think this might be a problem, but I don't understand why.

Additionally, I would highly appreciate if someone point out whether or not my copy/move ctor/assignments are implemented in the correct way.

Dree

With the copy-and-swap idiom, you don't need an additional move-assignment operator – Furtek

copy-and-swap covers both const& and &&. Either do copy-and-swap (only one overload taking param by value) or implement const& and && overloads for operator= – Overflight

Since you decided (why?) to do assignment by value, the argument can be passed through the move constructor. (By-value assignment only causes unnecessary copying. Don't do it.) – Morgenthaler

@Morgenthaler that's an idiom, whether it's a copy or a move depends on the usage – Furtek

@PiotrSkotnicki The usage is newGraph = std::move(graph); where I should invoke move assignment operator. I don't understand why I don't need additional move-assignment op with copy swap idieom. Without it I will call a regular assignment operator (which is probably not a good choice). – Dree

@Dree becase with the copy-and-swap idiom, the thing you swap with is move constructed first – Furtek

@PiotrSkotnicki Does the copy-and-swap idiom means that you manually copy object and then swap it, or it means that you will pass object by value (automatic copy) and then manually swap it? You are saying that in the first case I will need move assignment operator and in the second one I won't? Or I won't need it in both cases? – Dree

The answer to what you gave link explains it in good depth so it is really hard to understand what of it you don't understand. Did you read it with thought applied or just skimmed? – Strophic

@Dree depending on the value category of the argument expression, the parameter of operator= is either copy- or move-constructed – Furtek

@ÖöTiib Yes, I read it. This answer describes a rule of three in dept, and has a section "c++11" where the move constructor is described. It doesn't mention move assignment at all (except that rule of three became rule of four and a half). So, what I do not understand, when I should write and when I should not write move assignment operator. – Dree

@Dree it's "four and a half" because an assignment operator accepting a parameter by value can be used for copy- and move-assignment. this is because the argument expression can be either an lvalue or rvalue, and the parameter will be either move- or copy-constructed from that argument expression. as such, you don't need to provide a copy-assignment operator nor a move-assignment operator, because the one accepting parameter by value allows you to construct that parameter itself though move or copy construction. once the parameter is constructed,its content is swapped with the implicit object – Furtek

@Dejan: You need to look at when the actual copy or move is being done. With an assignment operator that takes its argument by value, the copy or move happens before the call to the assignment operator. The assignment operator itself doesn't copy or move, it merely swaps what it gets with its own state. Hence the decision whether to copy or to move, happens outside of the assignment operator, controlled by how you call it. – Hensley

Answer 1

You should have:

TUndirectedG& operator=(const TUndirectedG&);
TUndirectedG& operator=(TUndirectedG&&);

or

TUndirectedG& operator=(TUndirectedG);

Having both

TUndirectedG& operator=(TUndirectedG);   // Lead to ambiguous call
TUndirectedG& operator=(TUndirectedG&&); // Lead to ambiguous call

would lead to ambiguous call with rvalue.

Elroy