Finding and erasing a value from a std::vector holding std::map elements

Question

First, I have the following two objects, both filled with data:

std::vector<std::map<std::uint8_t, std::uint8_t>> x1;
std::vector<std::map<std::uint8_t, std::uint8_t>> x2;

My objective is to search inside x2 (by the key), checking if any value from x1 doesn't exist inside x2, and then erase it from x1.

I tried with the following code snippet, but to no avail (it doesn't compile!):

for (auto i = x1.begin(); i != x1.end(); ++i)
{
    auto it = std::find(x2.begin(), x2.end(), i);
    
    if (it == x2.end())
    {
        x1.erase(i);
    }
}

What am I doing wrong? Could you please share some insights on how to solve this problem?

To pass compilation: `auto it = std::find(x2.begin(), x2.end(), *i);` (missing star). — Jarod42, Aug 19 '20 at 18:51
Good point on the invalidation of `i` after erase. I tried with `std::set_intersection`, but getting wrong results! Yes, I don't have an issue sorting vector. — Xigma, Aug 19 '20 at 18:55
@Xigma -- The vectors should be sorted before calling `std::set_intersection`. — PaulMcKenzie, Aug 19 '20 at 19:12

Remy Lebeau · Answer 1 · 2020-08-19T20:04:47.107

There are several problems with your code:

std::find() searches for a single matching element, which in this case means you have to give it a std::map to search for. But you are passing in the i iterator itself, not the std::map that it refers to. You need to dereference i, eg:

auto it = std::find(x2.cbegin(), x2.cend(), *i);
When calling x1.erase(i), i becomes invalidated, which means the loop cannot use i anymore - not for ++i, not for i != x1.end(). You need to save the new iterator that erase() returns, which refers to the next element after the one being erased. Which means you also need to update your loop logic to NOT increment i when erase() is called, eg:
```
for (auto i = x1.cbegin(); i != x1.cend(); )
{
    auto it = std::find(x2.cbegin(), x2.cend(), *i);

    if (it == x2.cend())
        i = x1.erase(i);
    else
        ++i;
}
```

lastly, when using std::find(), you are comparing entire std::map objects to each other. If you are interested in comparing only the keys, try something more like this:

for (auto i = x1.cbegin(); i != x1.cend(); )
{
    const auto &m1 = *i:

    auto it = std::find_if(m1.cbegin(), m1.cend(),
        [&](const decltype(m1)::value_type &m1_pair) { // or (const auto &m1_pair) in C++14...
            return std::find_if(x2.cbegin(), x2.cend(),
                [&](const decltype(x2)::value_type &m2){ // or (const auto &m2) in C++14...
                    return m2.find(m1_pair.first) != m2.cend();
                }
            );
        }
    );

    if (it == m1.cend())
        i = x1.erase(i);
    else
        ++i;
}

@RemyLebeau Thanks a lot for the detailed and informative response. Indeed, I'm only interested in comparing the _keys_. — Xigma, Aug 19 '20 at 19:41
@RemyLebeau With the _keys_ comparison based approach, the compiler is throwing an error: "find': is not a member of 'std::vector — Xigma, Aug 19 '20 at 19:53
@Xigma sorry, forgot to take the `vector`s into account. I updated my example. — Remy Lebeau, Aug 19 '20 at 20:05
@RemyLebeau Thank you! Unfortunately, the compiler is still throwing errors on the updated version "error C2451: conditional expression of type '_InIt' is illegal". I'm using MS VC 2017. — Xigma, Aug 20 '20 at 00:41

Alexey S. Larionov · Accepted Answer · 2020-08-21T16:03:21.373

1

You can also go a little bit functional: Playground

#include <algorithm>
#include <functional>

// removes maps from x1, that are equal to none of x2 maps 
auto remove_start = std::remove_if(x1.begin(), x1.end(), [&](const auto& x1_map){ 
  return std::none_of(x2.begin(), x2.end(), 
    std::bind(std::equal_to(), x1_map, std::placeholders::_1)); 
});
x1.erase(remove_start, x1.end());

EDIT: To check keys only, change std::equal_to to a custom lambda

auto keys_equal = [](auto& m1, auto& m2){ 
  return m1.size() == m2.size() 
    && std::equal(m1.begin(), m1.end(), m2.begin(), 
      [](auto& kv1, auto& kv2){ return kv1.first == kv2.first; });
};

// removes maps from x1, that are equal to none of x2 maps 
auto remove_start = 
  std::remove_if(x1.begin(), x1.end(), [&](const auto& x1_map){ 
    return std::none_of(x2.begin(), x2.end(), 
      std::bind(keys_equal, x1_map, std::placeholders::_1)); 
  });
x1.erase(remove_start, x1.end());

edited Aug 21 '20 at 16:03

answered Aug 19 '20 at 20:00

Alexey S. Larionov

6,555
1
18
37

Interesting approach. Thank you! For whatever reason, it is not working for me! – Xigma Aug 20 '20 at 00:42
@Xigma might be that you don't use C++17 compilation – Alexey S. Larionov Aug 20 '20 at 09:44
I am using C++17 compilation. It is just that it it erasing everything from x1! – Xigma Aug 20 '20 at 15:27
1

@Xigma Strange, because it works in the playground I linked – Alexey S. Larionov Aug 20 '20 at 15:39
Indeed, the example you gave works fine! If it matters, this is how I'm pushing my data into the vector: `x1.push_back({ std::make_pair('a', 'b') });` – Xigma Aug 21 '20 at 12:11
@Xigma it shouldn't matter if your type is still `std::vector>`. Pretty sure `x1.push_back({ { 'a', 'b' } });` would work in C++17 too – Alexey S. Larionov Aug 21 '20 at 12:23
Right, that notation works too. It is quite bizarre that it is deleting the entire vector! – Xigma Aug 21 '20 at 12:46
@Xigma maybe you misspelled something or overrided `==` comparison of maps – Alexey S. Larionov Aug 21 '20 at 13:45
I'm not doing any of that! I have a question though, this `x1.erase(remove_start, x1.end());` technically would end up erasing the whole of x1. – Xigma Aug 21 '20 at 14:48
1

@Xigma it erases from `remove_start` to `x1.end()`, `remove_start` is initialized by `remove_if`, which would move all the soon-to-be-deleted maps in a tail of the vector – Alexey S. Larionov Aug 21 '20 at 14:50
1

@Xigma so `remove_start` should point to the first map to be deleted after `remove_if` moved all the unmatched maps – Alexey S. Larionov Aug 21 '20 at 14:51
I see! That makes sense. – Xigma Aug 21 '20 at 14:54
@ Alex Larionov I think the problem here is that the comparison isn't specific to the _key_, and thus why it is ending up removing all the elements of `x1` in my case. I'm only interested in checking the _key_. – Xigma Aug 21 '20 at 15:07
@Xigma well you can use [this](https://stackoverflow.com/a/8473603/8564999) question to compare keys only. Check my edit for updated version – Alexey S. Larionov Aug 21 '20 at 16:02
that's great! This is what I need. Thank you. – Xigma Aug 21 '20 at 16:54

Finding and erasing a value from a std::vector holding std::map elements

2 Answers2