Program crashes when using custom compartor for std::set

Question

I tried to use custom comparator in a std::set. When I insert cuisine "japaneses" in a variable bucketCuisines, I get error DEADLYSIGNAL.

But, If i eliminate the custom comparator cmp there is no problem. But of course my results are incorrect.

Input:

["FoodRatings","highestRated","highestRated","changeRating","highestRated","changeRating","highestRated"]
[[["kimchi","miso","sushi","moussaka","ramen","bulgogi"],["korean","japanese","japanese","greek","japanese","korean"],[9,12,8,15,14,7]],["korean"],["japanese"],["sushi",16],["japanese"],["ramen",16],["japanese"]]

Result:

[null, "kimchi", "ramen", null, "sushi", null, "ramen"]

class FoodRatings {
public:
    static bool cmp(pair<int, string> a, pair<int, string> b){
        if(a.first == b.first)
            return a.second < b.second;
        return a.first > b.first;
    }
    
    unordered_map<string, int> bucketFoods;
    unordered_map<string, string> bucketFoodNCuisine;
    map<string, set<pair<int, string>, decltype(cmp)*>> bucketCuisines;
    
    FoodRatings(vector<string>& foods, vector<string>& cuisines, vector<int>& ratings) {
        int n = foods.size();
        for(int i=0; i<n; i++){
            bucketFoods.insert(make_pair(foods[i],ratings[i]));
            cout << "1 ok\n";
            bucketFoodNCuisine.insert(make_pair(foods[i], cuisines[i]));
            cout << "2 ok\n";
            bucketCuisines[cuisines[i]].insert(make_pair(ratings[i], foods[i]));
            cout << "3 ok\n";
        }
    }
    
    void changeRating(string food, int newRating) {
        int oldRating = bucketFoods[food];
        bucketFoods[food] = newRating;
        string temp = bucketFoodNCuisine[food];
        bucketCuisines[temp].erase(bucketCuisines[temp].find(make_pair(oldRating, food)));
        bucketCuisines[temp].insert(make_pair(oldRating, food));
    }
    
    string highestRated(string cuisine) {
        return bucketCuisines[cuisine].begin()->second;
    }
};

Having trouble figuring this one out, especially as the input supplied in the question is not in the same form as described in the comments. — john, Jul 25 '22 at 10:30
please post a [mcve]. Include a `main` that calls the methods and produces the error — 463035818_is_not_an_ai, Jul 25 '22 at 10:37
A side note: for efficiency reasons, your comparison method should get `a` and `b` by reference (or even better a const refernce in this case). — wohlstad, Jul 25 '22 at 10:41

paddy · Accepted Answer · 2022-07-25T10:42:46.403

3

Your data type set<pair<int, string>, decltype(cmp)*> is an odd way to specify the comparator as a function pointer. But that's what it does, and the issue is likely because you never provide a valid comparator when constructing these sets.

When you add an entry to bucketCuisines, you're currently relying on the default constructor for this set. But that will initialize the comparator function pointer to nullptr. When you go to add stuff to the set, the null comparator is invoked and your program crashes.

The simplest way around this is to make the comparator a functor instead:

struct cmp {
    bool operator()(const pair<int, string>& a, const pair<int, string>& b) const
    {
        if(a.first == b.first)
            return a.second < b.second;
        return a.first > b.first;
    }
};

map<string, set<pair<int, string>, cmp>> bucketCuisines;

edited Jul 25 '22 at 10:42

answered Jul 25 '22 at 10:40

paddy

60,864
6
61
103

Beat me to the punch. C++11 changed things so that you cannot supply a comparator to any map constructor. This was news to me. Any idea why that change happened? – john Jul 25 '22 at 10:42
@john only the copy and move [constructors](https://en.cppreference.com/w/cpp/container/map/map) don't have an overload with the comparator. What changed in C++11 is that there are also overloads for supplying the allocator without changing the default comparator – Caleth Jul 25 '22 at 10:47
@Caleth also all constructors that take a comparator are listed as replaced in either C++11 or C++14 here https://en.cppreference.com/w/cpp/container/map/map. Do I miss something? – 463035818_is_not_an_ai Jul 25 '22 at 10:48
@Caleth My mistake, I meant C++14. – john Jul 25 '22 at 10:49
@463035818_is_not_a_number yes, they aren't replaced, the numbers correspond to multiple overloads. That's a rather confusing layout, I admit – Caleth Jul 25 '22 at 10:49
@Caleth ok, wasnt sure. Confusing indeed ;) – 463035818_is_not_an_ai Jul 25 '22 at 10:50
@Caleth Hmm, I tried this version of the OPs code `FoodRatings(vector& foods, vector& cuisines, vector& ratings) : bucketCuisines(cmp) {` and I got an error basically saying that I couldn't convert `cmp` to an allocator. So I'm still a bit confused. (though I see what you mean about the confusing layout). – john Jul 25 '22 at 10:52
The `bucketCuisines` map is not the one that uses the comparator. The type of the comparator's parameters does not match the key for the map (which is a string). It's used by the set being stored in the map. Some typedefs would make this code a bit nicer to read. – paddy Jul 25 '22 at 10:56
@paddy Oh right,. Thanks, problems solved. – john Jul 25 '22 at 10:57
Thanks, @paddy I understand your solution and meaning. It`s old-solution. But i find C++ New solution in here -> https://stackoverflow.com/questions/2620862/using-custom-stdset-comparator <- I checked this solution, but it doesn`t help me. I used comparator only about SET, Not map. But Why the error occured? I can`t understand still – Eunno An Jul 25 '22 at 11:07
1

@463035818_is_not_a_number I've re-numbered that page – Caleth Jul 25 '22 at 11:14
@Caleth Wow, big improvement. – john Jul 25 '22 at 11:19
@EunnoAn Paddy's second paragraph explains why the error occurs. What about that explanation confuses you? – john Jul 25 '22 at 11:21
I have confusion about null comparator. Why null-comparator? I declared the function cmp, not null-function. I confused the relation between null-comparator and default constructor. – Eunno An Jul 25 '22 at 11:45
You told the `std::set` that its comparator is a _function pointer_ that looks like `cmp` or _any other_ function with the same signature. You must then provide the function pointer you wish to use to the set's constructor. You don't do this, and so the function pointer is null. – paddy Jul 25 '22 at 11:56
@paddy ```bool cmp(int a, int b) { return ...; }```If this function , I can use ```std::set s(cmp);``` this way either ```std::set s(&cmp);```this way. But My source code has errored. Sorry for my low-level understanding. – Eunno An Jul 25 '22 at 12:03
@paddy I understand your comment above. But i can`t understand why these solution`s wasn`t working for me (Solution : https://stackoverflow.com/questions/2620862/using-custom-stdset-comparator) – Eunno An Jul 25 '22 at 12:05
Post a new question. Nobody can know what you did wrong without seeing it. And this comments section is already ridiculously long and off topic. All I can say is what you did in _this_ question, which I already explained in my answer. – paddy Jul 25 '22 at 12:08

Program crashes when using custom compartor for std::set

1 Answers1