CODEWITHSUNDEEP
c++liststl
FerranMG
FerranMG

Reputation: 181

Where's the bug in this simple std::list?

I can't understand where's the problem with this very simple list I've been testing. The idea is to get the item at position i in a list. I know usually I wouldn't use a list to do that. However, this works when I set item = 11, item = 12 and item = 13 (output would be, respectively, at position {1, 2, 3} there's the item {11, 12, 13}), BUT it does not work when I set item = 10, as the output is at position 0 there's the item 6.

int main(void)
{
    list<int> L;
    L.push_back(10);
    L.push_back(11);
    L.push_back(12);
    L.push_back(13);

    int item = 10;
    int pos;

    list<int>::iterator it = moveToItem(item, L, pos);

    cout << "at position " << pos << " there's the item " << *it;
}

list<int>::iterator moveToItem(int item, list<int> L, int& pos)
{
     pos = 0;
     list<int>::iterator it = L.begin();

     while(*it != item)
     {
         it++;
         pos++;
     }
     return it;
}

Upvotes: 1

Views: 249

Answers (3)

hmjd
hmjd

Reputation: 121961

A copy of the list L is being made when moveToItem() is called so the returned iterator is referring to an item of a list that has been destructed. Pass the list by reference instead:

list<int>::iterator moveToItem(int item, list<int>& L, int& pos)
                                                //^

You should also protect against going past the end() of the list in the while condition, before dereferencing it.

If this is not an exercise consider using the STL algorithm std::find() and std::distance() instead:

#include <iterator>
#include <algorithm>

std::list<int>::iterator it = std::find(L.begin(), L.end(), 41);
if (it != L.end())
{
    std::cout << "at position "
              << std::distance(L.begin(), it)
              << " there's the item "
              << *it
              << "\n";
}

Upvotes: 7

Christian Rau
Christian Rau

Reputation: 45948

You are taking the list by-value. Therefore the returned iterator is an iterator into the local L argument of the function and is therefore invalid once the function returned (and L got destroyed). You should take L by reference:

list<int>::iterator moveToItem(int item, list<int> &L, int& pos)

Performance-wise it's not the best idea to take such a possibly large datastructure as a list by-value, anyway.

Upvotes: 0

tiguero
tiguero

Reputation: 11537

You should pass a reference to your list when calling the list<int>::iterator moveToItem(int item, list<int> L, int& pos) currently a copy of your list made.

So your method should be list<int>::iterator moveToItem(int item, list<int>& L, int& pos) instead. You can keep the body of your method the same.

Upvotes: 0

Related Questions