When insert a new item in stl map, occurs an overwrite to first element

Question

Firstly, I have the following code:

map<char*, char*> records;

// id, value are buffer variables to store the text that returns from `strcpy()` 
char *id = (char*)malloc(sizeof(char*) * 20);
char *value = (char*)malloc(sizeof(char*) * 20);

sql->open(dbFile);
sql->query("SELECT * FROM categories");
while(sql->fetch() != SQLITE_DONE){

    // copy the text that returns from `sql->getValue`, then convert that text from `const unsigned char*` to `const char*` 
    strcpy(id, reinterpret_cast<const char*>(sql->getValue(0)));
    strcpy(value, reinterpret_cast<const char*>(sql->getValue(1)));

    // insert the values that in `id`,`value` as new item
    records.insert(pair<char*, char*>(id, value));           
}
free(id);
free(value);

The idea of the previous code is copy the text that returns this method sql->getValue(0) by using strcopy(), then convert that text from const unsigned char* to const char*, then store these text in the id, value variables, then insert id, value as a new item. The previous steps occur on each data that gets from database.

Clarify the idea more:
For example, There are three rows fetched from database (fruits, vegetables, sweets).
Naturally, when insert these three data into the map container, the number of items that in the container will be three items first(fruits), second(vegetables), third(sweets).

in my case, when checking the number of items, I find there is one item only, and the item is sweets.

Now, I amazed, where is the rest of items ?

The key you are using for your `std::map` is **not** the `id`, it is the `char*` to the `id`. I suggest you use `std::string` as I think you have memory leaks too. — Galik, Jan 02 '15 at 03:47
You were told in your last question that a map of `char*` was a bad design. It can be done but you'll have a memory management mess, just like in this code. — Blastfurnace, Jan 02 '15 at 03:53
@Galik: Thanks, exactly, I was know that, but wasn't know how to solve the problem without using string type. — Lion King, Jan 02 '15 at 03:57
@Blastfurnace: Yes, but I'm use the stl map only For the purpose of simulate the associative array. — Lion King, Jan 02 '15 at 04:00
@LionKing: Every time you call `insert()` you pass the __same addresses__ for the key/value pair. That's all the `std::map` sees, two pointers. It doesn't follow the pointers to look at the strings or make copies. You might modify the contents of the `char` arrays between calls but the `std::map` doesn't know that. And after the loop you `free()` the memory which leaves dangling pointers in the `std::map`. — Blastfurnace, Jan 02 '15 at 04:03
@LionKing: Have you read a good C++ book? [Try one of these, they're fascinating!](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) — Blastfurnace, Jan 02 '15 at 04:22
You are speedy when voting down, but you are slow when voting up. — Lion King, Jan 04 '15 at 14:15

Wintermute · Accepted Answer · 2015-01-02T03:57:29.903

1

You are overwriting the same slabs of memory three times and storing the pointers to the slabs three times. As far as the map is concerned, it's the same value every time because the pointers never change.

It appears that what you really want is to have a map of strings, not pointers. This will be much easier if you use std::string:

#include <string>

...

map<string, string> records;

string id, value; // size of allocation will be handled by the string class.

sql->open(dbFile);
sql->query("SELECT * FROM categories");
while(sql->fetch() != SQLITE_DONE){
  // string has an assignment operator from C-style strings
  id    = reinterpret_cast<const char*>(sql->getValue(0));
  value = reinterpret_cast<const char*>(sql->getValue(1));

  // more readable way of writing the insert. id, value will be copied into
  // the map.
  records[id] = value;
}

// freeing unnecessary; string class handles it when id, value go out of scope.

This also has the advantage of making the code exception-safe (at least the strings) without a try-catch-block. The destructor of std::string will handle the cleanup regardless of how the scope is left.

edited Jan 02 '15 at 03:57

answered Jan 02 '15 at 03:51

Wintermute

42,983
5
77
80

Thank you, but there is no way except using string type? – Lion King Jan 02 '15 at 04:07
@LionKing: You could allocate memory for __every__ string you insert in the `std::map` and be sure to manually release that memory at the right time and only once. Or you could let `std::string` correctly manage that resource for you... – Blastfurnace Jan 02 '15 at 04:12
There is, but you don't want to use it. Seriously, it'd be worse than cancer^Wvery bad things. You'd have to allocate new memory for new C-strings inside the loop, copy the values into them, and store those in the map. You'll have to do manual cleanup of all those C-style strings. You'll have to do error handling for all those allocations. And if the map is to remain sorted, you'll have to supply a comparison functor to the map that works with C-style strings. You really don't want to do that sort of thing. Trust me and everyone else on this. – Wintermute Jan 02 '15 at 04:12
@Blastfurnace: Thanks, I think I will use the string type. – Lion King Jan 02 '15 at 04:21

When insert a new item in stl map, occurs an overwrite to first element

1 Answers1