set::find finds an element that does not exist

Question

I have the following Edgeclass:

class Edge {
public:
int src, dest;

bool operator== (const Edge &edge) const {
    return ((src == edge.src) && (dest == edge.dest)) || ((src == edge.dest) && (dest == edge.src));
}

bool operator<(const Edge& edge) const {
    return !(((src == edge.src) && (dest == edge.dest)) || ((src == edge.dest) && (dest == edge.src)));
}

Edge(int src, int dest) {
    this->src = src;
    this->dest = dest;
}
};

The point of overriding the < operator is when I try to find an edge in a set Edge(0, 1) should be equal to Edge(1, 0). However, the following test code fails to do so and std::find returns an edge that doesn't even exist:

Edge edge(0, 3);
set<Edge> test;
test.insert(Edge(3, 1));
test.insert(Edge(3, 0));
auto a = test.find(edge);
cout << a->src << " " << a->dest << endl;

This will strangely print out 2 0. I can't figure out why and I'm new to C++ any help is appreciated.

Your code contains undefined behavior as `operator<` does not implement strict ordering between edges. — Mestkon, Jun 08 '20 at 13:22
your `operator<` is completely illogical and violates the strict weak ordering assumption on the set's comparator — Sopel, Jun 08 '20 at 13:22
@Mestkon I thought of that as well but what should I do in order to achieve the functionality I mentioned? — Gökberk Şahin, Jun 08 '20 at 13:23
First fix your < operator it should give the items an order. yours is simply !=. Second the find can give you the end iterator of the set (if nothing was found). Attempting to access it results in undefined behavior. Thats the reason you get wierd numbers. — Taron, Jun 08 '20 at 13:25
[Pro Tip] Use `std::tie` to make comparison operators. A less than function can be written like `bool operator <(const class_name& lhs, const class_name& rhs) { return std::tie(lhs.mem1, lhs.mem2, ..., lhs.memN) < std::tie(rhs.mem1, rhs.mem2, ..., rhs.memN); }` — NathanOliver, Jun 08 '20 at 13:25
"Not equals" is not "less than". What about when it's "greater than"? — Asteroids With Wings, Jun 08 '20 at 13:34

score 7 · Accepted Answer · answered Jun 08 '20 at 13:30

7

You currently don't have a valid Compare for std::set, so your program has undefined behaviour.

Here is one that is compatible with your ==

bool operator<(const Edge& edge) const {
    return std::minmax(src, dest) < std::minmax(edge.src, edge.dest);
}

This can also be used to simplify your ==

bool operator==(const Edge& edge) const {
    return std::minmax(src, dest) == std::minmax(edge.src, edge.dest);
}

answered Jun 08 '20 at 13:30

Caleth

52,200
2
44
75

1

Didn't know [std::minmax()](https://en.cppreference.com/w/cpp/algorithm/minmax), makes Stephan's code a one-liner. Neat. – Peter - Reinstate Monica Jun 08 '20 at 13:40
Great answer. Might I suggest also advising OP to add !=, >=, <= and >? (The principle of least surprise). (I know it works without them) – Yakk - Adam Nevraumont Jun 08 '20 at 13:42
@Yakk-AdamNevraumont I prefer inheriting `boost::totally_ordered` for C++11, and would switch to `operator <=>` for C++20 – Caleth Jun 08 '20 at 13:44
@Yakk-AdamNevraumont Good idea but I really don't need them, this code will be only run for proving some kind of induction step at a graph based game. Only data structure I need is set so < will be enough. – Gökberk Şahin Jun 08 '20 at 13:45
@gokberk sure, but they are easy and good practice. The first 4 times you write them there will be bugs and you'll write them poorly. If you pay attention and try to improve them each iteration, by the time you aren't making toy code your >= might suck less. ;) – Yakk - Adam Nevraumont Jun 08 '20 at 23:34

score 1 · Answer 2 · answered Jun 08 '20 at 13:32

There are two issues in your code.

First, you do not check whether test.find() returns a valid edge; note that find returns end() if no element was found.

Second, your <-operator does not implement a strict ordering, it actually just defines a !=. To overcome this, I'd normalize each edge such that the lower node is always treated as the start; then decide based on the starting nodes, and only if they are equal, consider the destination nodes:

class Edge {
public:
int src, dest;

bool operator== (const Edge &edge) const {
    return ((src == edge.src) && (dest == edge.dest)) || ((src == edge.dest) && (dest == edge.src));
}

bool operator<(const Edge& edge) const {
//    return !(((src == edge.src) && (dest == edge.dest)) || ((src == edge.dest) && (dest == edge.src)));

    int thisSrc = std::min(src,dest);
    int thisDest = std::max(src,dest);
    int eSrc = std::min(edge.src,edge.dest);
    int eDest = std::max(edge.src,edge.dest);

    if (thisSrc < eSrc) {
        return true;
    } else if (thisSrc > eSrc) {
        return false;
    } else {
        return thisDest < eDest;
    }
}

Edge(int src, int dest) {
    this->src = src;
    this->dest = dest;
}
};

#include <set>

int main() {
  Edge edge(0, 3);
  std::set<Edge> test;
  test.insert(Edge(3, 1));
  test.insert(Edge(3, 0));
  auto a = test.find(edge);
  if (a == test.end()) {
     std::cout << "edge not found." << std::endl;
  } else {
    std::cout << a->src << " " << a->dest << std::endl;
  }
}

Output:

3 0

Thank you for taking the time to write the code, I appreciate it. @Caleth's one-liner worked for me as well as your code. — Gökberk Şahin, Jun 08 '20 at 13:53

set::find finds an element that does not exist

2 Answers2