# List iterator not decrementable (C++)

This topic is 2943 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Ugh. Ok. So this code worked fine in Xcode for Mac. I port this thing over to Windows and MSVC 8 decided he didn't like it. So, I'm having iterator problems. I know WHAT is causing the problems. I just don't know a better way to re-write this!

setActiveLevel(const std::string level)	{		bool exists = false;				std::map<std::string, boost::shared_ptr<LevelManager> >::iterator i;		std::list<std::string>::iterator j;		std::list<std::string>::reverse_iterator k;				for(i = mManagers.begin(); i != mManagers.end(); ++i)		{			if(i->first == level && !levelNames.empty())			{				exists = true;								for(j = levelNames.begin(); j != levelNames.end(); ++j)				{					std::cout << (*j) << std::endl;										if((*j) == level)					{						if(++j != levelNames.end())						{							nextLevel = (*j);						}					}				}										//Now find the previous level				for(k = levelNames.rbegin();k != levelNames.rend(); ++k)				{					if((*k) == level)					{						if(++k != levelNames.rend())						{							previousLevel = (*k);						}					}				}				break;			}		}				if(!exists)		{			std::cout << "Error setting the active level! Name does not exist!\n";		}				lastVisitedLevel = activeLevel;		activeLevel = level;				std::cout << "nextLevel: " << nextLevel << std::endl;		std::cout << "previousLevel: " << previousLevel << std::endl;		std::cout << "lastVisitedLevel: " << lastVisitedLevel << std::endl;		std::cout << "activeLevel: " << activeLevel << std::endl;	}

The goal of this code, is when I set the level (by name. i.e. "Level1") it finds stores the other two variables, nextLevel and previousLevel.

So if I can't increment or decrement passed my current iteration without getting that error from MSVC, how else can I do this?

Thanks for the help in advanced, guys. :)

##### Share on other sites
So how is MSVC not happy. You are getting runtime errors, because it is checking your iterator usage very rigorously?

Anyway, your code is a bit bizarre.

Why do you loop over each item in a map, instead of using the find member function (that's what the map was invented for)?

The rest of the logic is hard to follow too. What happens if you don't find "level" in the "levelNames"? If the list is guaranteed to contain the name, why do you even need to search for it, if you already know what the next level is going to be.

As to the backwards search, why do you have to search again? Don't you just want to look at the previous item (if level is not the first item in the list)?

This is how you'd find the level in the map:
i = mManagers.find(level);if (i != mManagers.end()) // success

This is how you'd find the level in the list:
j = std::find(levelNames.begin(), levelNames.end(), level);if (j != levelNames.end()) //success

And to access the previous item
if (j != levelNames.begin()) {    --j;    //access previous item now}

##### Share on other sites
Well one problem was easy to spot. Here's one of your loops with the bits not related to the problem stripped out:
for(j = levelNames.begin(); j != levelNames.end(); ++j){	if (some_condition)		if (++j != levelNames.end())			some_statement;}

As you can see, the iterator j is incremented twice each time through the loop if some_condition is true. A problem then occurs when some_condition is true for the last item. j will be incremented and will then equal end(), next the foor loop increment case runs and increments j past end() BOOM![dead], before the continuation condition of the for loop would be evaluated.

It occurs to me that you probably don't intend to skip any item that follows the one where some_condition is true. You simply want to see if you are on the last item without any iterator modification. The same problem exists in your k loop.
There is no doubt far better ways of doing what you are trying to accomplish, but that's all I have time for sorry.

I would be unhappy about Xcode not catching such a bug for me rather than being unhapppy that VS2008 did find the bug.

##### Share on other sites
Yeah, I should be iMalc. You are right. I think I read somewhere that list in Xcode my be circular whereas MSVC is only linear.

Thanks for the tips guys. Especially you visitor. You're right on the implementation. I don't know what I was thinking. I know better to use ::find. I may have been rushing.

##### Share on other sites
It took me a while to figure out what you were really trying to do.

The variable names help. :) But you're making things much, much too complicated.

The idea is:

Find the current level.If it is not the last in the list:  Assign the next element of the list to nextLevel.If it is not the first in the list:  Assign the previous element of the list to prevLevel.

We don't need two separate loops for that. We don't need any loops, because there is a standard library algorithm for the first part.

Also, we should probably check that the current level is actually in the list before we try any of the rest. Your existing algorithm would not set 'nextLevel' or 'previousLevel' in this case, but would set the 'exists' flag and break out of the loop. This is probably not what we want.

We can also simplify loops (reduce indentation) with judicious use of 'continue'.

That gives us something like:

void setActiveLevel(const std::string level) {	bool exists = false;			std::map<std::string, boost::shared_ptr<LevelManager> >::iterator i;	std::list<std::string>::iterator j;	std::list<std::string>::reverse_iterator k;	typedef std::list<std::string>::iterator level_it;			for (i = mManagers.begin(); i != mManagers.end(); ++i) {		if (i->first != level || levelNames.empty()) { continue; }		level_it begin = levelNames.begin(), end = levelNames.end();		level_it current = std::find(begin, end, level);		if (current == end) { continue; }		exists = true;		if (current != begin) { previousLevel = *(current - 1); }		if (current + 1 != end) { nextLevel = *(current + 1); }		break;	}			if (!exists) {		std::cout << "Error setting the active level! Name does not exist!\n";	}			lastVisitedLevel = activeLevel;	activeLevel = level;			std::cout << "nextLevel: " << nextLevel << std::endl;	std::cout << "previousLevel: " << previousLevel << std::endl;	std::cout << "lastVisitedLevel: " << lastVisitedLevel << std::endl;	std::cout << "activeLevel: " << activeLevel << std::endl;}

But now we notice that 'levelNames' is a constant, and if it's empty, it will be empty on every iteration. So what we really want to do is check that first, and then we are simply searching through the map of managers to see if the 'level' is present. Each time through the loop, we do no real work if the level does not match, so really the loop has no purpose except to check for presence in the map. So we do that separately, too; and we again do it using built-in standard library functionality. (To search for a key in a std::map, we use the map's .find() member function, not the global std::find(), which would look for a key-value pair.)

We also should not be doing I/O here (I assume that stuff at the bottom is just for debugging); if there is an error, we should throw an exception or return some kind of error code. We currently do not have any normal value to return, so the natural thing to do is just "return whether successful".

Finally, we can clean up a couple of unused variables, pass the level name by const reference, etc.

// This doesn't really need any comments; I'm just putting them in to show you// how simple this really is.bool setActiveLevel(const std::string& level) {	// Check if there is a manager for the named level.	if (mManagers.find(level) == mManagers.end()) { return false; }	// Check that the level name is in the list.	// We don't actually need a separate check for an empty list, because	// an empty list cannot contain the level name. ;)	level_it begin = levelNames.begin(), end = levelNames.end();	level_it current = std::find(begin, end, level);	if (current == end) { return false; }	// All the checks passed, so we can set the level successfully.	// Set the previous/next level names, if possible.	if (current != begin) { previousLevel = *(current - 1); }	if (current + 1 != end) { nextLevel = *(current + 1); }	// Finish up.					lastVisitedLevel = activeLevel;	activeLevel = level;	return true;}

##### Share on other sites
Wow. Holy crap. I knew I was over complicating things but I didn't realize I could shrink it down by that much. I really need to get better at using iterators. >_>

Thanks a bunch. I've definitely learned a lot from this little thing!

##### Share on other sites
Not iterators so much as algorithms - in two senses: writing out what you are trying to do in a way that makes things simple and keeps separate tasks separate; and leveraging the standard library algorithm implementations.

##### Share on other sites
For those of you who stumble upon this topic. Std::list is not what you need. I had to change it to std::vector. Lists do not allow random element access (like *(current+1) ) but vectors do.

:)

##### Share on other sites
Quote:
 Original post by Maverick ProgrammerFor those of you who stumble upon this topic. Std::list is not what you need. I had to change it to std::vector. Lists do not allow random element access (like *(current+1) ) but vectors do. :)

Oh it was a list? Darn, I don't read goods :/

##### Share on other sites
Heh, I missed that. :)

Of course, we can still increment and decrement with std::list. So we can do something like:

// rest of my awesomeness as before	if (current != begin) { level_it prev = current; prev--; previousLevel = *prev; }	level_it next = current; next++;	if (next != end) { nextLevel = *next; }

and std::find does not require random access, so no problem there. :)

Or instead of making copies and incrementing/decrementing, you could use std::advance ;)

1. 1
2. 2
3. 3
Rutin
21
4. 4
frob
18
5. 5

• 33
• 13
• 10
• 10
• 12
• ### Forum Statistics

• Total Topics
632568
• Total Posts
3007119

×