Jump to content
  • Advertisement
Sign in to follow this  
Maverick Programmer

List iterator not decrementable (C++)

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

If you intended to correct an error in the post then please contact us.

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 this post


Link to post
Share on other sites
Advertisement
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by Maverick Programmer
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.

:)

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

Share this post


Link to post
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 ;)

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!