0

Sorry, I know there are questions where people ask how to remove duplicates, but I thought this warranted a new thread since there is something weird coming out of my compiler.

vector<string> removeDuplicates(vector<string> vector)
{
    for (vector<string>:: const_iterator it = vector.begin(); it != vector.end(); ++it)
    {
        for (vector<string>:: const_iterator sit = vector.begin(); sit != vector.end(); ++sit)
        {
            if(it != sit)
            {
                if(*it == *sit)
                {
                    vector.erase(it);
                }
            }
        }
    }
return vector;
}

I don't understand why it's giving me errors. I am kinda new to C++, so the question might sound kinda dumb. Is it because I am looping on the same vector?

enter image description here

OnTheFly
  • 2,059
  • 5
  • 26
  • 61

4 Answers4

4

You do not want to erase 1 item at a time. Use the remove-erase pattern which will erase all the elements that should be removed at 1 time, instead of several erase calls. In this case (as you appear to want to remove all duplicate elements), you can copy the vector to a std::set (which will not allow copies) and then assign it back.

The error message appears to be showing a bug in your implementation. std::vector::erase is supposed to be able to take a const_iterator (see quote from the standard below), but the error message you are seeing is indicating your implementation does not have it defined that way.

§ 23.3.6.5

iterator erase(const_iterator position);
iterator erase(const_iterator first, const_iterator last);

You will also get undefined behavior with your current implementation as the iterator is invalidated once it is erased, so the it++ will not be valid in the loop condition.

If you switch to the more efficient removal methods, this would likely fix that problem as well.

Set Method

std::vector<std::string> vec;
// fill vector with duplicates
std::set<std::string> sVec(vec.begin(), vec.end()); // copies the vector into a set, duplicates removed
vec.assign(sVec.begin(), sVec.end()); // pushes the data in the set back to the vector

Remove/Unique-Erase Method (assuming order does not matter)

std::vector<std::string> vec;
// fill vector with duplicates
std::sort(vec.begin(), vec.end());
vec.erase(std::unique(vec.begin(), vec.end()), vec.end());
Zac Howland
  • 15,777
  • 1
  • 26
  • 42
  • remove-erase pattern? do you have a link? – OnTheFly Jan 24 '14 at 05:42
  • if(*sit == *it) v.erase(std::remove(v.begin(), v.end(), sit), v.end()); //would this work? – OnTheFly Jan 24 '14 at 05:44
  • @OnTheFly The edit has a version of it using `std::unqiue` (which returns an iterator to the first element that should be removed). The same can be done with `std::remove` and `std::remove_if`. – Zac Howland Jan 24 '14 at 05:45
  • @OnTheFly Most of the time, no. The algorithms already have the loop written, you just tell it where to begin and end. – Zac Howland Jan 24 '14 at 05:47
  • k i got it working, but it only works if i take the original and make a duplicate vector and then use that vector to fill the set, do you know why? – OnTheFly Jan 24 '14 at 06:08
  • @OnTheFly How are you attempting to fill the `std::set` (that is, can you show the code you are trying to use)? – Zac Howland Jan 24 '14 at 14:45
1

I think, the conditions should be :

    if(it != sit)
    {
        if(*it == *sit)
        {
            sit = vector.erase(sit)
        }
        else
        {
            ++sit;
        }
    }

And don't increment sit in for loop

Remove the constness of the iterator which you are erasing. Make sit as vector::iterator.

sajas
  • 1,599
  • 1
  • 17
  • 39
  • why should you use else ++sit? – OnTheFly Jan 24 '14 at 05:17
  • If *it and *sit are not equal then go for the next sit. But if they are equal we should just erase the current sit element and use the iterator returned by erase. We should not increment the sit at that point – sajas Jan 24 '14 at 05:19
  • C:\Qt\Tools\QtCreator\bin\tutorial01\assignment1\main.cpp:68: error: no matching function for call to 'std::vector >::erase(std::vector >::const_iterator&)' i2 = avector.erase(i); ^ – OnTheFly Jan 24 '14 at 05:21
  • @OnTheFly Remove the constness of sit. vector:: iterator sit – sajas Jan 24 '14 at 05:22
1

When an item in the vector is erased , use the valid iterator which the erase() returns. like , iterator = vector.erase(iterator)

Also, when you happen to erase the last item in the vector, doing a ++iterator will not work.

This link may help vector erase iterator

Community
  • 1
  • 1
vathsa
  • 303
  • 1
  • 7
-1

Do this: 

it = vector.erase(it);
cppcoder
  • 22,227
  • 6
  • 56
  • 81