0

I'm trying to implement an undirected graph:

template<typename NodeType, typename Edge>
class Graph {
public:
    Graph() = default;

    Graph& Add(const Edge &edge, const NodeType &from, const NodeType &to) {
        auto node = graph.emplace(Node(from)); //passing ‘const std::set<Graph<City, Train>::Node, std::less<Graph<City, Train>::Node>, std::allocator<Graph<City, Train>::Node> >’ as ‘this’ argument discards qualifiers
        (node.first)->edges.emplace_back(edge);

        node = graph.emplace(Node(to));
        (node.first)->edges.emplace_back(edge);

        return *this;
    }

    void PrintGraph() const {
        for(const auto &node : graph) {
            cout << node.id << ":" << endl;
            for(const auto &edge : node.edges)
                cout << "-> " << edge << endl;
        }
    }
private:
    class Node {
    public:
        Node(const NodeType &val): id(val) {}
        Node(const Node &other): id(other.id) {
            copy(other.edges.begin(), other.edges.end(), edges);
        }

        bool operator<(const Node &other) const {
            return id < other.id;
        }

        NodeType id;
        vector<Edge> edges;
    };

    set<Node> graph;
};

This won't compile due to my (node.first)->edges.emplace_back(): error: passing ‘const std::vector<Train, std::allocator<Train> >’ as ‘this’ argument discards qualifiers [-fpermissive] which I don't understand:

  • member function Add() returns a reference to itself since I want to chain it. It cannot be a const reference since the function modifies its class members.

  • member function Add() cannot be const as it could add a Node or Node's edge.

  • it's possible that graph.emplace() returns pair<set::const_iterator,bool> so I try verifying it by changing to pair<set<Node>::const_iterator,bool> node = but then a whole new set of problems arises: type/value mismatch at argument 1. I thought set::emplace return a pair of iterator to new/existing element and true/false depending on whether new element was added. Where am I wrong here?

So what's wrong with my Add()?

I use my templated graph class on the following classes:

class City {
public:
    City(const string &cityName): name(cityName) {}
    City(const City &cpy) = default;

    friend ostream& operator<<(ostream &os, const City &city) {
        os << city.name;
        return os;
    }
    bool operator<(const City &other) const {
        return this->name < other.name;
    }
    bool operator==(const City &other) const {
        return this->name == other.name;
    }

    string name;
};

class Train {
public:
    Train(const string &trainName): name(trainName) {}
    Train(const Train &cpy) = default;

    friend ostream& operator<<(ostream &os, const Train &train) {
        os << train.name;
        return os;
    }
    bool operator<(const Train &other) {
        return this->name < other.name;
    }
    bool operator==(const Train &other) const {
        return this->name == other.name;
    }

    string name;
};

The class is used this way:

Graph<City,Train> network;

network.Add(Train("train 1"), City("city 1"), City("city 2")).Add(Train("train 2"), City("city 1"), City("city 2"));
Sorry
  • 532
  • 2
  • 6
  • 19
  • You call `Add` on a `const` object. That means `Add` also have to be `const` qualified. If you can't do that, then you need to refactor the code to not use a `const` object. – Some programmer dude Apr 28 '18 at 07:53
  • @Someprogrammerdude which const object would that be? – Sorry Apr 28 '18 at 07:53
  • Since you don't show us a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve), no one but yourself knows what object you call `Add` on. – Some programmer dude Apr 28 '18 at 07:54
  • @Someprogrammerdude just added usage. – Sorry Apr 28 '18 at 07:55
  • 1
    @Someprogrammerdude If that were the problem, wouldn't it complain about the call to `Add()`, not the call `(node.first)->edges.emplace_back()`? – Barmar Apr 28 '18 at 07:56
  • You pointed out the problem. `std::set::emplace` returns a constant iterator as the first element. Anything accessed through it is `const`. It's [more iffy](https://godbolt.org/g/QFRas5) why your type doesn't work, but documentation points out that the return type uses `iterator`, not `const_iterator`. I imagine they're allowed to differ even if they're both constant. – chris Apr 28 '18 at 07:58
  • I'll bet it's complaining about both. – Barmar Apr 28 '18 at 07:58
  • 2
    When you place a value into a `set`, you're not allowed to modify it because it may destroy the sort order in which the `set` keeps elements to speed operations (the sorting is termed a *"class invariant"* because the class needs to keep it true at all times). If you're "key" for comparisons really doesn't include the edges, then consider making them `mutable`, or changing to a `std::map`. – Tony Delroy Apr 28 '18 at 07:59
  • @TonyDelroy of course! Completely forgot about that. So the solution is to use `map` instead? – Sorry Apr 28 '18 at 08:00
  • In this case, since `edges` is not accessible externally and does not affect the order, I agree with Tony Delroy: I would also consider marking `edges` `mutable`. –  Apr 28 '18 at 08:01
  • Or `unordered_map` (or just a plain `vector`) if you don't need any ordering. – Some programmer dude Apr 28 '18 at 08:01

0 Answers0