Noob problem with std::map

[Krice]

I wanted to try std::map for the first time!

It's int, Message_Text* pair. I'm looking for id from a message data to create map item like this:

map[id]=new Message_Text(text location of id here);

Then checking out with 'find' if the key exists (because if you just use [] to access map it will create a new object if no key found with that id).

The problem is that the text position is wrong so it's doing something to data (even hangs when trying couple of times) or it's not somehow pointing to proper id. I must be missing something simple, right?

[Cfyz]

Yeah. Most likely you've messed something up with the pointers. Hard to tell without the code, but out of the two entities shown -- std::map and 'new Message_Text' -- the latter one is waay more error prone. In modern C++ using raw pointers is a pretty bad practice. You need a damn good reason to use '*' nowadays.

[Krice]

In modern C++ using raw pointers is a pretty bad practice.

I'm not using it in any other way than placing it in the map (I guess?). So it's not even raw.

[Cfyz]

Pointer is either wrapped (e. g. smart pointer) or raw. Raw gets garbage value every chance it gets: at declaration, if not careful with container, when deleting via one of the several copies, etc. and generally is a pain to look after, besides some more architectural drawbacks. After years of practice you start to get around all this intuitively, but in every aspect C-style memory management in C++ just complicate things.

I'm fairly positive it is some (quite possible very simple) mistake in manual memory management. Make that map contain actual values (yep, copies of) instead of pointers to, and no way in hell std container will get you unpredictable results. Well, unless you still messing the heap/stack completely elsewhere =)

[reaver]

1. what's "text location of id"?
2. "no key found with that id" does not make sense. Your id is your key, the value is the Message_Text

[Krice]

Make that map contain actual values (yep, copies of) instead of pointers to, and no way in hell std container will get you unpredictable results

Let's just say the exact same instance created with 'new' works fine with std::vector. But since there are "gaps" in the ids sometimes I thought using map would be a better option. On the contrary I have bad experiences placing actual objects to std containers, because the way it makes the copy. I guess the way std containers are made is really stretching the imagination of C/C++ anyway. So I always put pointers in lists and vectors (not including simple cases like vector of ints) and it's working fine, because the container doesn' t have to copy the objects.

I mean, you CAN put pointers in a map, right? So why isn't this working then?

[Krice]

Pointer is either wrapped (e. g. smart pointer) or raw. Raw gets garbage value every chance it gets: at declaration, if not careful with container, when deleting via one of the several copies, etc. and generally is a pain to look after

I have to disagree. If you follow the ownership paradigm it's really easy to handle "raw" pointers or object instances as I call them. Nothing is lost or no pointer is re-assigned with that paradigm. People who do get in trouble with raw pointers are not doing it right.

[Krice]

Well as always the problem was not in the std::map itself, but in two of my own routines. First when I was creating messages from raw data it was interpreting the data so that there were always at least two versions of the text, even the data had only one. I think this was the reason for hanging, because the second message was garbage. The second bug was in the processing of the text data which made only one letter of message appearing, because I returned false and true in wrong order.

Here's how I handled the insertion:

Code: [Select]

Message_Text *mt=new Message_Text(ptr+t);
texts.insert(std::make_pair<int, Message_Text*>(id, mt));

I guess insert returns something if the id is already in the map, maybe I could check that or it doesn't matter, since the insert just fails if double ids exists.

The map is created in a constructor and items are deleted in the destructor so it's not "raw". And items are searched with find to disallow creation of new empty items.

[Cfyz]

If you follow the ownership paradigm it's really easy to handle "raw" pointers or object instances as I call them. Nothing is lost or no pointer is re-assigned with that paradigm.

Well, that's kind of true. What I want to point out is that std::shared/weak/unique_ptr and value semantics are ownership paradigm personified. Making silly mistakes becomes much harder.

[Krice]

What I want to point out is that std::shared/weak/unique_ptr and value semantics are ownership paradigm personified.

For some reason I don't like C++ smart pointers. I think I have even heard about strange failure of smart pointers in some situations, which sounds quite right, because C++ was never designed for them. I like the direct approach, because then you can be sure it either works or doesn't. Also, often when raw pointers fail it's easy to notice and find the bug. At least with Visual Studio's debug mode.

[TheCreator]

After years of practice you start to get around all this intuitively, but in every aspect C-style memory management in C++ just complicate things.

I guess that everybody should use what they are most accustomed to. After years of practice in using raw pointers you probably won't just switch to smart pointers in a week (even if you can start programming Java in a week). Also, refactoring a big project that has been built on raw pointers is definitely not an option.

I have fixed thousands of memory problems in my life. Most of them were not silly mistakes. They were about not understanding how my own program works (yes, this happens in big projects). Smart pointers can not replace thinking. They're just syntactic sugar that often hides the real problem, which is lack of understanding the code. Once you fully understand the code, you know who is the owner of the memory and managing it becomes easy.

[Xecutor]

Here's how I handled the insertion:

Code: [Select]

Message_Text *mt=new Message_Text(ptr+t);
texts.insert(std::make_pair<int, Message_Text*>(id, mt));


This is potential memory leak. You need to check if an item with this id already exists in the map and only if it doesn't create new Message_Text object.
Or at least delete previous instance if you want to overwrite it.

Code: [Select]

  Message_Text *mt=new Message_Text(ptr+t);
  auto it = texts.find(id);
  if( it != texts.end() ) {
    delete it->second;
    it->second = mt;
  }
  else {
    texts.insert(std::make_pair<int, Message_Text*>(id, mt));
  }

I'll add that in case of pointer wrapper you don't need to care about this

[Krice]

This is potential memory leak.

Yes.. so I modified the source somewhat different way, informing if duplicates in message data exist.

Code: [Select]

Message_Text *mt=new Message_Text(ptr+t);

pair<map<int, Message_Text*>::iterator, bool> rv;
rv=texts.insert(std::make_pair<int, Message_Text*>(id, mt));

if (rv.second==false)
printf("Id %d already exists (map size is now: %d).\r\n",
id, (int)texts.size());


[Cfyz]

I guess that everybody should use what they are most accustomed to. After years of practice in using raw pointers you probably won't just switch to smart pointers in a week (even if you can start programming Java in a week). Also, refactoring a big project that has been built on raw pointers is definitely not an option.

The latter bit about refactoring a big project is undeniably true. The first two points, however, not so.

Using only what you are accustomed to without thinking is dangerous. Tools evolve and practices change. It's okay to prefer particular alternative, but ignoring years of evolution in computer science is not wise. I probably can guess where you're coming from. AFAIK you primarily use C so this must look like choice between proven, natural approach (manual memory management) and somewhat alien one (automatic memory management). We, however, are talking about C++, where it is as natural and have direct language support. If you are not accustomed to something like that, you are ignoring an integral part of the language. Think about arguing against using structures in C just because you technically can program without them (reductio ad absurdum).

You can switch to smart pointers in a week, unless the project too big or you are pressured for time, etc. but we are talking about fairly small programs here (roguelikes), and most of them are written from scratch (though it may not be the case here). Just as you can switch from homebrew list/map implementations to std::list/std::map in a week. One of the biggest things about smart pointers is they are almost drop-in replacement of a careful version of manual pointer management. Just as std::list is a list does exactly the same as the list implementation you might come up yourself (plus a few other features), smart pointers do exactly the same thing you would end up doing with your pointers yourself. Besides some really specific cases, by using standard library you lose nothing and still gain some.

I have fixed thousands of memory problems in my life. Most of them were not silly mistakes. They were about not understanding how my own program works (yes, this happens in big projects). Smart pointers can not replace thinking. They're just syntactic sugar that often hides the real problem, which is lack of understanding the code. Once you fully understand the code, you know who is the owner of the memory and managing it becomes easy.

I find this argument far beside the point. Nothing can replace thinking, be that smart pointers or your hard-earned coding practices. And standard library is not a syntactic sugar. Once again, std::list is not a syntactic sugar over prev/next fields, it is a carefully written and extensively tested double-linked list implementation, written and tested in a way not many can do. In the same manner, std::shared_ptr is very robust implementation of reference-counted pointer container. At best, you'll re-implement the same.

Also note that just because some things might look different, it does not mean they are not one and the same. Both

Code: [Select]

class A {A(); ~A();};and

Code: [Select]

A* alloc_a();
void free_a(A* a);are essentially the same. Lack of 'class' keyword does not make latter less object-oriented. Similarily, when you are manually managing the memory by keeping track of the pointers, counting references or carefully transferring the ownership, you are doing essentially the same thing as smart pointer will do but without language and compiler support. There are cases where it may be the right approach (e. g. C where there is no language support to begin with) but projects in modern C++ are rarely the ones.

[TheCreator]

AFAIK you primarily use C so this must look like choice between proven, natural approach (manual memory management) and somewhat alien one (automatic memory management). We, however, are talking about C++, where it is as natural and have direct language support.

My previous post might indeed sound like I'm a C programmer, but in fact I've started from Visual Basic and later switched to C++ (like 15 years ago). I've been using stuff like std::string or std::map from the very start, so it's not that I don't value automatic memory management. It's enough to use any raw WinAPI function to see why std::string was such a giant step forward in programming. However, I can't see a single reason why to use smart pointers (apart from one legitimate use of auto_ptr, but that's another story, I guess).

It seems that you are talking about C++11 here. Well, everyone knows it has cool features, but unfortunately some people are stuck in the stone age with their Visual Studio 2005 and can't enjoy that coolness. All the smart pointer support that C++03 has is the infamous auto_ptr, which is not so smart, as you probably know. One day I'll buy a new computer, install new Visual Studio version and port my project to C++11, but before that happens, I would like to learn why smart pointers are as great as they say. There must be something more than just a call to delete at the right moment. Something that would justify the fact that they introduced 3 complicated pointer classes, deprecating a simple

Code: [Select]

type* variable.

You can switch to smart pointers in a week, unless the project too big or you are pressured for time, etc. but we are talking about fairly small programs here (roguelikes), and most of them are written from scratch (though it may not be the case here).

Well, maybe, I've done such things before. Like switching from char* to std::string in my early C++ days, or switching to Unicode several years ago. However, I would like to see how it will improve the development process, otherwise it would be a lost week for me (and definitely a lot of fun ).

And standard library is not a syntactic sugar. Once again, std::list is not a syntactic sugar over prev/next fields, it is a carefully written and extensively tested double-linked list implementation, written and tested in a way not many can do. In the same manner, std::shared_ptr is very robust implementation of reference-counted pointer container. At best, you'll re-implement the same.

OK, I've gone too far saying it's a syntactic sugar. As of shared_ptr's usefulness, I'm not convinced. My biggest problem with reference counting are memory leaks. If I forget to free an object which is tied to a complex hierarchy of objects, all those objects fail to release their dynamically allocated memory and I get a giant leak report. It takes ages to investigate which object has caused a leak. Does shared_ptr address this problem? If it does, I'm already motivated to start using it .