tags:

views:

165

answers:

2

I've got this function:

    template<typename T>
    void Inventory::insertItem(std::vector<T>& v, const T& x)
    {
        std::vector<T>::iterator it; // doesn't compile
        for(it=v.begin(); it<v.end(); ++it)
        {
            if(x <= *it) // if the insertee is alphabetically less than this index
            {
                v.insert(it, x);
            }
        }
    }

and g++ gives these errors:

src/Item.hpp: In member function ‘void
yarl::item::Inventory::insertItem(std::vector<T, std::allocator<_CharT> >&, const T&)’:  
src/Item.hpp:186: error: expected ‘;’ before ‘it’  
src/Item.hpp:187: error: ‘it’ was not declared in this scope

it must be something simple, but after ten minutes of staring at it I can't find anything wrong. Anyone else see it?

+15  A: 

Try this instead:

typename std::vector<T>::iterator it;

Here's a page that describes how to use typename and why it's necessary here.

Cogwheel - Matthew Orlando
That did it. Thanks.
Max
+1 for concise answer and a link for the detailed explanation.
+6  A: 

What you are doing is inefficient. Use a binary search instead:

#include <algorithm>

template <typename T>
void insertItem(std::vector<T>& v, const T& x)
{
    v.insert(std::upper_bound(v.begin(), v.end(), x), x);
}
FredOverflow
+1 -- beat me to it. It's probably also worth mentioning that since the current code doesn't break out of the loop after doing the insertion, it'll typically insert extra copies of the new item where they're not desired.
Jerry Coffin
@Jerry: Even worse, if the vector's size equals its capacity, `insert` will invalidate all iterators obtained prior to the insertion, so `++it` will lead straight into undefined behavior land.
FredOverflow
@Fred: good point.
Jerry Coffin
+1 (but wish I could give +10). Looking at the code a second time, the build error is but one minor problem compared to the runtime behavior of the code.
+1 - Thanks for pointing that out!
Max