find with char* variable doesnt work

Question

I'd like to know why I have a memory error with this:

The problem appears on char* value = aMap.find(keync)->second

If I put manualy char* value = "key0" it works!!!

using std::map;
map <char*, char*> aMap;

void search(const char* key) {
    const int LEN = strlen(key);

    char* keync = new char[LEN];

    for (int i= 0; i < LEN; i++) {
       keync[i] = key[i];
    }

    char* value = aMap.find(keync)->second;

    printf("%s", value);

    delete[] keync;
}

int _tmain(int argc, _TCHAR* argv[])
{
    a["key0"] = "value0";
    search("key0");

    return 0;
}

What aren't you using `std::string`? The conversion from a string literal to `char*` is deprecated, don't do it. At the least use `const char*`. Also, the map is going to be comparing pointer values, not strings, you need to provide a custom comparator. (Or just use `std::string`, again.) Stop doing manually memory management, wrap things up. (`std::vector` exists for this, or, again, `std::string`.) And lastly, do remember the terminator for C-strings. (Maybe [a book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) would be helpful.) — GManNickG, Aug 30 '10 at 04:28
@Okami: You need to make one, then. Resource use needs to be separated from resource management. Make a class with the Big Three, then use that class, don't try to mix them together. You can wrap it around `std::vector`. And if you don't have that, there's the Big Three class you need to make. The moment you have a need to manually delete a resource outside of managing it, you've done something wrong. — GManNickG, Aug 30 '10 at 04:33
@GMan: You need to put your comments into an answer, so we can vote it. — Martin York, Aug 30 '10 at 05:26
@Martin: Meh. :) I like to let other people get reputation points sometimes. — GManNickG, Aug 30 '10 at 05:42

score 2 · Answer 1 · answered Aug 30 '10 at 04:24

2

You need to add 1 to the length of the array:

char* keync = new char[LEN+1];

You're null terminating outside the string you allocated.

(Also, are you initialising aMap?)

answered Aug 30 '10 at 04:24

No one in particular

2,682
2
17
20

Good catch on the off-by-one. The `map` initialization is fine. It just default-constructs it. – Matthew Flaschen Aug 30 '10 at 04:27
I'm initializing the map. My char* is only "key0" Should I put the terminator '\0' ? – okami Aug 30 '10 at 04:27
@Okami, "key0" is equivalent to `{'k', 'e', 'y', '0', '\0'}`. So the null-terminator is already there. – Matthew Flaschen Aug 30 '10 at 04:30
ok, by your explanation with or without putting '\0' the size will be LEN+1. Weird C stuff :-/ – okami Aug 30 '10 at 04:38
@Okami, no, if you put `"key0\0"` (there's no reason to do this), it would be, `{'k', 'e', 'y', '0', '\0', '\0'}`. The `strlen` would still be the same (4 in this example), since that's the number of characters before the first NUL-terminator. You have to allow room for a single NUL-terminator, for a total size of LEN + 1 (5 in the example). – Matthew Flaschen Aug 30 '10 at 04:43

score 1 · Answer 2 · answered Aug 30 '10 at 04:55

As others pointed, you are much better off using std::string for this. Now, for the actual problem why you are not able to find the string is because you are storing pointers in the map i.e. the key to the map is a pointer variable. You inserted a char* in to the map but when you are trying to find you are doing a new again. This ia a totally different pointer (although the string value they point is same) hence your lookup will fail.

score 0 · Answer 3 · answered Aug 30 '10 at 04:18

0

One obvious issue is the:

delete keync;

Because you used new [], it should be:

delete[] keync;

answered Aug 30 '10 at 04:18

Matthew Flaschen

278,309
50
514
539

ok I missed that, but the problem occours on: char* value = aMap.find(keync)->second; – okami Aug 30 '10 at 04:22

score 0 · Answer 4 · answered Aug 30 '10 at 04:24

You are better using std::string instead of char*. Another plus of using std::string is that you will avoid memory leaks.

The other solution will be to provide map with a comparator function, otherwise they will not compare the content of each char*, but instead the address pointed. The following example was adapted from Sgi's std::map documentation:

struct comp
{
  bool operator()(char* s1, char* s2) const
  {
    return strcmp(s1, s2) < 0;
  }
};

map<char*, char*, comp> stringMap;

radman · Answer 5 · 2010-08-30T05:12:50.423

Well look for starters you should drop all the char* stuff and use std::string, this would probably make your problem go away.

FYI:
keync[LEN] = '\0'; // this is wrong and will be one past the end of the allocated array

The copying of the key parameter is wrong. Try this on for size:

using std::map;
map <char*, char*> aMap;

void search(const char* key) {
    const int LEN = strlen(key);

    char* keync = const_cast<char*>(key);

    char* value = aMap.find(keync)->second;

    printf("%s", value);
}

int main(int argc, char** argv)
{
    aMap["key0"] = "value0";
    search("key0");

    return 0;
}

The problem you were having is because you were allocating keync as strlen(key) which doesn't count the null terminator. In your copy loop you were then overwriting the null terminator with the last char of key.

The whole idea of copying the input string is wrong and I have replaced it with a const cast in my solution as it makes more sense (as far as that goes).

Ok I corrected that: keync[LEN] = '\0'; The problem still goes on. — okami, Aug 30 '10 at 04:32

score 0 · Answer 6 · answered Aug 30 '10 at 08:00

using std::map;
map <char*, char*> aMap;

first of all, this map won't compare strings (inside of search) but addresses. So basicly you won't find anything with std::map::search by typing string literals.

void search(const char* key) {
    const int LEN = strlen(key);

    char* keync = new char[LEN];

    for (int i= 0; i < LEN; i++) {
       keync[i] = key[i];
    }

at this moment you have unterminated string, but it doesn't matter in your code

    char* value = aMap.find(keync)->second;

here you're performing search by comparing pointers values (addresses), so returned map iterator is invalid (it's equal aMap.end()), so either it has null or unallocated pointer as second member

    printf("%s", value);

    delete[] keync;
}

int _tmain(int argc, _TCHAR* argv[])
{
    a["key0"] = "value0";
    search("key0");

    return 0;
}

I hope it explains you why should you use std::string instead of char *

find with char* variable doesnt work

6 Answers6