function return map value => iterator crashs prg, why?

[cn00by]

hello,
i have an easy example that crashs.

but i did not know the reason.

the iterator != end() check doesnt work,
it prints me the 3rd element, but only 2 elements in the map.

#include <string>
#include <iostream>
#include <map>

class test_map
{
	public:
		test_map(void){internal_map.insert(std::pair<int, std::string>(1,"test"));internal_map.insert(std::pair<int, std::string>(2,"test"));}
		std::map<int, std::string>	get_map(void){return internal_map;};
	
	private:
		std::map<int, std::string>	internal_map;
};

int main(void)
{
	test_map a;
	std::cout << "a size: " << a.get_map().size() << std::endl;
	
	int i=0;
	for(std::map<int, std::string>::iterator it=a.get_map().begin(); it != a.get_map().end(); ++it)
		std::cout << "i: " << i << " first " << it->first << " second " << it->second << std::endl;
 }

i get the following output:
test_map.exe
a size: 2
i: 0 first 1 second test
i: 0 first 2 second test
i: 0 first 1852397404 second
... crash
can someone explain me that?

thanks

[Repeater]

Your get_map function returns a copy of the internal map. So every time you're checking to see if it's time to finish ( it != a.get_map().end() ) you're using a whole new, completely different map object, so you're comparing iterators to different maps.

Don't return a copy of the map. Return a const ref to it, or some other such way of using the actual internal map, rather than a copy of it.

Obviously in a real class you wouldn't want to be just handing people direct access to internal variables, but for learning, this is all very educational.

[cn00by]

hello,
in my opinion both function calls:
- a.get_map().begin() AND
- a.get_map().end()

creates two locale instances, and thats the reason why the iterator end doesnt work,
because first instance end != second instance end.

right?

[TheIdeasMan]

Hi

Try this for loop, it is a safer way of iterating through a container, easier too

auto TheMap = a.get_map();
		
		for (auto&& item : TheMap) {
		    std::cout << "i: " << i << " first " << item.first << " second " << item.second << "\n";
		}

The auto&& is forwarding reference, you could have const auto& instead. item is one one of the items in the container.