Sign in to follow this  
Khatharr

for each, in - copying std::map elements?

Recommended Posts

Having a little time to mess around with coding I sat down and opened my input engine and found that it's no longer responding to keypresses. (I swear the thing was working when I archived it.)

 

I followed it in the debugger and manually simulated a keypress, but something is wrong with this blasted VS2010 'for each' loop:

 

void KeyMap::update() {
  for each(auto key in m_keys) {
    key.second.update();
  }
}

 

There's only one element in the std::map 'm_keys'. I follow the debugger into the update() call and have it set the object state to indicate that the key is pressed. When I come back out of the function keys.second has the correct state, but if I examine m_keys, the element there does not have the correct state. So apparently this loop is copying the std::pair objects from the map here? I disabled the copy ctor for the object's class and sure enough I get a compiler error. If I comment out the loop body I still get the error, so it's the loop making the copy, not the std::pair.

 

The MSDN docs for the loop make the cryptic comment 'When identifier is a tracking reference, you can modify the element.'

 

Gosh, thanks, MS. How the hell do I get it to work by reference rather than trying to copy?

 

I suppose I can just do it the old fashioned way, but I do like the simplified syntax.

 

Any advice would be appreciated. Thank you for your time anyone that responds.

Edited by Khatharr

Share this post


Link to post
Share on other sites

That would be 'pointer to' in that case, wouldn't it?

...

 

Ah no, I get it. Because it's on the left.

Compiler doesn't like it.

1>c:\users\khatharr\desktop\khatengine\khatengine\cl_keymap.cpp(13): error C2662: 'VirtualKey::update' : cannot convert 'this' pointer from 'const VirtualKey' to 'VirtualKey &'
1> Conversion loses qualifiers

Wait a minute, why the hell is the VK marked const?

Let me see if I can track that down.

Okay, now I'm really confused. m_keys is declared:

std::map<unsigned int, VirtualKey> m_keys;

But this thing is trying to const it for that loop. What's up with that?

 

...

 

Same problem with this:

 

void KeyMap::update() {
  std::map<unsigned int, VirtualKey>::iterator iter = m_keys.begin();
  iter->second.update();
  return;
  //for each(auto& key in m_keys) {
    //key.second.update();
  //}
}
Edited by Khatharr

Share this post


Link to post
Share on other sites

The description here of the for each method has been corrected at the bottom of the page:

for each returns read only objects There is an error under the section Remarks in paragraph 1.

It is not possible to update the elements in a collection by updating the element that is returned by the for each command. The for each command returns an object by value, not by reference.

 

 

This should not lead to a const error:

void KeyMap::update() {
  std::map<unsigned int, VirtualKey>::iterator iter = m_keys.begin();
  iter->second.update();
  return;
}
}

Share this post


Link to post
Share on other sites

I think you mixed C++ and C# syntax.
 
The C++11 for range syntax is

 

for(element : container)

so try

for(auto& key : m_keys) {
key.second.update();
}

[edit]

Fixed code blocks

Edited by Rattrap

Share this post


Link to post
Share on other sites
No, the problem is that MSVC (maybe not 2012 but not sure of that) if they support for each at all, only support the for each ( ... in ...) syntax. And apparently even that is only half the deal because the const-only requirement is not a property of proper for each loops.

Share this post


Link to post
Share on other sites

VS 2012 definitely supports range based for-loops. I don't know if any VC before 2010 supported that weird proprietary "for each( ... in...)" syntax, so using it seems like a really bad idea if you ever plan to change/update your compiler. Apparently even if sticking with 2010, considering the annoying const-ness limitation.

 

The more or less "portable" alternative to using proper C++11 in VS2012 is the BOOST_FOREACH macro in boost or simply typing those few extra expressions for a regular for-loop.

Share this post


Link to post
Share on other sites
So I presume there's some good reason you can't move to VS2012 and get to use the standard C++11 range-for (along with saving a lot of other grief as well)?

In VS2010 it's possible to use the for_each algorithm, or write a small template of your own for slightly more readability, while sticking to the standard:
// standard library
for_each(v.begin(), v.end(), [&](type& x){
...
});
// own template
foreach(v, [&](type& x){
...
});
I think that's got most of the usability, if not the performance, of the VS nonstandard extension. If you want both performance and usability, BOOST_FOREACH.

Share this post


Link to post
Share on other sites

Try this

for(auto& iter : m_keys)
{
     iter.second.update();
}

 

 

 

That would be 'pointer to' in that case, wouldn't it?

...

 

Ah no, I get it. Because it's on the left.

 

Yes and no...

int a;
int& a_reference = a;
int* a_pointer = &a;

The "&" will always be on the left.

Share this post


Link to post
Share on other sites

First: God dammit, Microsoft!

 

Thank you for pointing that out, Ashaman.

 

Second.

 

Yes and no...

int a;
int& a_reference = a;
int* a_pointer = &a;

The "&" will always be on the left.

 

I meant on the left side of the (implied) assignment. Thank you for the clarification, though. :)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this