5

What I am trying to do is to make it possible to use std::unordered_set with my custom class Vector2 - including the possibility to search for objects of such class that are already in a set.

Let me give more details. The header of my class Vector2, including the custom hash table, is the following:

Class Vector2
{
private:
    static int lastID;
    const int id;
    int x;
    int y;

public:
    Vector2(int _x, int _y);
    ~Vector2();

    bool Vector2::operator==(const Vector2& other) const;

    int getId() const;
    int getX() const;
    int getY() const;
};

namespace std
{
    template<>
    struct hash<Vector2>
    {
        size_t
            operator()(const Vector2& obj) const
        {
            return hash<int>()(obj.getId());
        }
    };
}

The implementation of the memeber functions of such class is trivial:

int Vector2::lastID = 0;

Vector2::Vector2(int _x, int _y) : id(lastID++)
{
    x = _x;
    y = _y;
}

int Vector2::getId() const
{
    return id;
}

int Vector2::getX() const
{
    return x;
}

int Vector2::getY() const
{
    return y;
}

bool Vector2::operator==(const Vector2& other) const
{
    if (x != other.x || y != other.y) return false;
    return true;
}

Then, my main functions looks like the following:

std::unordered_set<Vector2> mySet;
mySet.insert(Vector2(1, 2));
mySet.insert(Vector2(3, 11));
mySet.insert(Vector2(-5, 0));

Vector2 whatToLookFor(1, 2);
if (mySet.find(whatToLookFor) != mySet.end())
{
    std::cout << "Found it!" << std::endl;
}
else
{
    std::cout << "Nothing found." << std::endl;
}

However, while I would expect the output to be Found it!, it is actually Nothing found. It means, while the Vector2 objects Vector2(1, 2), Vector2(3, 11) and Vector2(-5, 0) are inserted in mySet, they later are not found when searching inside such set.

What am I doing wrong?

Andy
  • 71
  • 1
  • 6
  • You are implementing `hash` wrong. We require that, if `a == b` then `hash(a) == hash(b)`. – kennytm Feb 21 '17 at 06:20
  • In addition to SingerOfTheFalls answer, beware that the copy constructor of `Vector2` will create another `Vector2` object with the same ID. That may or may not be what you want. – Martin Bonner supports Monica Feb 21 '17 at 06:59
  • @MartinBonner Thanks for pointing that out! Indeed, in my ideal scenario, all `Vector2` objects that have same `x` and same `y` would have the same `id`, but I guess that is of minor importance for this particular case, right? – Andy Feb 21 '17 at 07:06

1 Answers1

3

The search operations in an unordered_set are largely dependent on the hash value of the elements, requiring that given the hash function h: if A == B, then h(A) == h(B).

When a search is performed, the hash function is used to determine the bucket to which the element belongs. After that, if there are multiple elements in the bucket, additional checks are performed to compare these elements with each other. This may be done by comparing the elements directly, re-hashing them again, or using other techniques.

Your class has two data members, and apparently you expect to "find" an element by these exact members (i.e. you expect that if A.x == B.x && A.y == B.y, then A == B and vice-versa). However your hash function only hashes based on the id of the elements, ignoring their other members, that's why it doesn't work as you expect.

The solution would be to rewrite your hash function to utilize the value of the fields. You also might want to check this documentation page.

SingerOfTheFall
  • 29,228
  • 8
  • 68
  • 105
  • That is exactly what I want to find. So, what I don't get is the following. In my hash overloading, instead of returning `obj.getId()` I suppose I could then return a combination of the object's attributes I'm really interested in - `x` and `y`. However, how do I guarantee uniqueness of such combination? I've seen examples that do something like `return obj.getX()+obj.getY()`, but seems insufficient – Andy Feb 21 '17 at 06:40
  • @Andy, well I believe there are multiple ways to achieve that, but I feel that's more like a math problem than a programming problem. Anyway, you may find answers to [this](http://stackoverflow.com/questions/919612/mapping-two-integers-to-one-in-a-unique-and-deterministic-way) questions useful. – SingerOfTheFall Feb 21 '17 at 06:43
  • 2
    Your description isn't quite right. `unordered_set` requires that `A==B` implies `h(A) == h(B)` (which the OP doesn't have). `h(A) == h(B)` does *not* imply that `A == B` (the counter example would be a hash collision - which is unfortunate for performance, but not catastrophic for correctness). – Martin Bonner supports Monica Feb 21 '17 at 06:57
  • Following up on @MartinBonner's comment, `operator==` *is* used in search operations to resolve hash collisions, so search operations are not *solely* based on the hash values. – juanchopanza Feb 21 '17 at 07:01
  • 1
    @juanchopanza, I meant to say that the operations to find a bucket are based solely on hash values, but you're right, it's poorly worded. I'll edit it. – SingerOfTheFall Feb 21 '17 at 07:03