0

I am facing some strange problem in C++ execution. My project is big where I have used pass by reference in several places but I am showing you a sample of the issue you can test easily.

I wrote this simple program and tested it below under GCC version: v4.9.2 and v5.4.0. I get different behavior esp. when passing std::string with and without reference. In this program, I simply add two entries to a map and find the value for a key. The problem is with map::find(..). I expect consistent behavior irrespective of any gcc compiler version used.

GCC version: 4.9.2 output: FOUND, FOUND (tested on Raspberry Pi3, http://cpp.sh)

GCC version: 5.4.0 output: NOT FOUND, FOUND (tested on Ubuntu v16.04)

Why is such a case? Is there something wrong with the program or is that a compiler bug somewhere? The program below should compile as is.

// program.cpp
// Compile using: "g++ -o program program.cpp"

#include <iostream>
#include <string>
#include <map>
#include <stdio.h>
using namespace std;

std::map<const char*,int> mymap;

bool FindWithoutPassByRef(const std::string id_)
{
    const char* id = id_.c_str();

    std::map<const char*, int>::iterator it;
    it = mymap.find(id);

    if (it != mymap.end())
    {
        cout <<"FOUND";
        return true;
    }

    cout <<"NOT FOUND";
    return false;
}

bool FindWithPassByRef(const std::string& id_)
{
    const char* id = id_.c_str();

    std::map<const char*, int>::iterator it;
    it = mymap.find(id);

    if (it != mymap.end())
    {
        cout <<"\nFOUND";
        return true;
    }

    cout <<"\nNOT FOUND";
    return false;
}

int main(int argc, char *argv[])
{
    printf("gcc version: %d.%d.%d\n",__GNUC__,__GNUC_MINOR__,__GNUC_PATCHLEVEL__);

    const std::string key1 = "key1";
    const std::string key2 = "key2";

    mymap[key1.c_str()] = 50;
    mymap[key2.c_str()] = 60;

    FindWithoutPassByRef(key1); // should print FOUND
    FindWithPassByRef(key1);    // should print FOUND

    cout<< endl;
}

I would have expected FOUND and FOUND under any gcc compiler. See the example running fine under GCC v4.9.2 here or add the above code on cpp.sh (uses v4.9.2). For compiler v5.4.0, you can test this on Ubuntu or elsewhere appropriately.

Guy Coder
  • 24,501
  • 8
  • 71
  • 136
Sammy
  • 257
  • 2
  • 8
  • @slava, it would be nice if you could share the link of the post along with marking it as duplicate. – Sammy Jan 18 '17 at 15:49
  • Thanks, got it here (http://stackoverflow.com/questions/4157687/using-char-as-a-key-in-stdmap). But that post is not really a duplicate, as my post relates to behavior with const char* as key in map + with inconsistent working on different compiler versions. – Sammy Jan 18 '17 at 15:51
  • 1
    link is there on top, right under subject. Your problem is exactly the same - you do not use proper comparator. – Slava Jan 18 '17 at 15:51
  • 1
    This can happen if 4.9.2 uses the "copy on write" optimisation on strings (which is not allowed since C++11). I have no idea whether g++ ever did that, though. – molbdnilo Jan 18 '17 at 16:16
  • 1
    The fact it works on gcc < 5.x is due to the std::string implementation of gcc (up to 5.x as you found), GCC used to alloc a single buffer and share it between all the strings that used it until one of the strings change ( copy on write), newer gcc versions, to comply to c++ 11 standard moved to an std::string based on the small string optimization. – gabry Jan 18 '17 at 16:18
  • Thanks @gabry for checking this out. Very much appreciate it. But i guess, using const char* pointer as KEY should never be used. What do you think? +1 One of the best answer to my question. – Sammy Jan 18 '17 at 16:47
  • @Sammy there are use cases also for const char * keys, for instance I used them in a map of 100 millions records of 8 to 10 bytes string with strict memory constraints and the std::string overhead was not negligible, I used a single memory allocation for all the keys and all the values, and obviously strcmp() as compare function. – gabry Jan 20 '17 at 08:10
  • @gabry, can you show me specifically then how you could avoid this problem? I believe using const char* would be way more efficient for larger number of records than std:string. Pls. correctmeif i am wrong. Any sample example would be great. – Sammy Jan 21 '17 at 08:21

1 Answers1

4

You're putting pointers in a map and trying to match them to other pointers. This only compares the "memory address" held by those pointers; it does not compare the things that they point to (i.e. your string data).

When your pointers are pointers to the data from a single std::string, odds are good that they are the same pointers (because the data is in the same place). However, when your pointers are to different strings, the odds of that are very small.

Don't put char pointers in maps. Compare actual strings instead.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • 1
    There may even be UB here because the copy constructor is not a `const` member function, invalidating the result of `.c_str()`. I wouldn't really expect a difference in behaviour here but it may help explain why observable behaviour is being changed apparently due to a copy-elision optimisation. Doesn't really matter anyway. Like I said, just don't do it! – Lightness Races in Orbit Jan 18 '17 at 15:44
  • Thanks, it makes sense. However, could you tell me why would there be inconsistent behavior across different compiler versions (as specified above)? – Sammy Jan 18 '17 at 15:44
  • 1
    @Sammy: I could probably work it out exactly but I'm not inclined to spend time on it because there is no value in doing so. Perhaps someone else has more time to spare :) – Lightness Races in Orbit Jan 18 '17 at 15:45
  • @Sammy I bet it depends if actual copy is created or eliminated by optimization. But it is pretty much useless to find exact reason. – Slava Jan 18 '17 at 15:47
  • Ok, so basically what you are trying to suggest is to use non-pointer type for the 'key field'. Using a pointer for the key field is not a good practice. Am I right? The value field however could be a pointer. – Sammy Jan 18 '17 at 15:47
  • 2
    @Sammy: Even having a pointer in the value field is suspicious in many cases (and sometimes you do want a pointer in the key field, when it is actually pointers that you want to map) but, yes, that's basically right. I can't think of a valid use case for putting the result of `std::string::c_str()` in a container at all. – Lightness Races in Orbit Jan 18 '17 at 15:47
  • 1
    @Sammy read answer of duplicate, you can use pointer with proper comparator. But your code is dangerous - you insert pointer of local object into global map. – Slava Jan 18 '17 at 15:48
  • Yes but don't.. – Lightness Races in Orbit Jan 18 '17 at 15:49
  • @ Lightness Races in Orbit: Thanks for the insight. This mostly does answer my question. However, just curious to see if someone does get hands-on why there is this consistently 'inconsistent' compiler behavior. I will then mark the question as answered ;) – Sammy Jan 18 '17 at 15:58
  • 1
    Well found and answered by @gabry: "The fact it works on gcc < 5.x is due to the std::string implementation of gcc (up to 5.x as you found), GCC used to alloc a single buffer and share it between all the strings that used it until one of the strings change ( copy on write), newer gcc versions, to comply to c++ 11 standard moved to an std::string based on the small string optimization." – Sammy Jan 18 '17 at 16:47
  • Marked as answered. Thanks LightnessRacesinOrbit and @gabry for best answers.. – Sammy Jan 18 '17 at 16:49