nlohmann / fifo_map

a FIFO-ordered associative container for C++
MIT License
179 stars 77 forks source link

find() adding a key #2

Closed bernau84 closed 7 years ago

bernau84 commented 7 years ago

While combines find() followed by add/insertion of item strange thing happens. Code

nlohmann::fifo_map<std::string,uint8_t> pe;
for(int i=0; i<1000; i++){

        std::string key = "custom_";
        key += std::to_string(i);

        //if(0 == pe.count(key)){
        if(pe.find(key) == pe.end()){

            pe[key] = (i+128) & 0xFF;
            std::cout << "OK"  << std::endl;
        } else {

            std::cout << "WRONG" << std::endl;
            auto i = pe.find(key);
            std::cout << key << " matches " << i->first << "!!" << std::endl;
            std::cout << key << " counts " << pe.count(key) << "!!" << std::endl;
            break;
        }
}  

This test won't pass (or enter "WRONG" condition) on my PC (tested on 32bit mingw530 and 64bit g++ 5.4.0) in third iteration.

Up to my knowledge it fails because new key is added into fifo_map::m_keys already with fifo_map::find(name) call. This new key has first = name, second = 0. Second value should be timestamp > 0, so it is not valid key. I don't know why, perhaps it could be desired behavior BUT when I want than add key of such name it has no effect because of usage insert() in fifo_map_compare::add_key.

I used simplest workaround and replace insert() by operator[]() but I'm on serious doubts if it is the right correction (despite it works).

void fifo_map_compare<class Key>::add_key(const Key& key) {
        keys->operator[](key) = timestamp++; //bernau84 workaround
        //keys->insert({key, timestamp++});
}

To be honest - the same code runs as should on online compiler http://melpon.org/wandbox/permlink/l2f2Qxhq95qVKRgE. This confuses me totally.

bernau84 commented 7 years ago

My previous proposal was mistake, it causes more problems than solves. After some more digging I find the root of problem in fifo_map_compare::operator() New slow but working solution could be:

        // look up timestamps for both keys
//        const auto timestamp_lhs = keys->operator[](lhs);
//        const auto timestamp_rhs = keys->operator[](rhs);

        //bernau84 fix
        const auto timestamp_lhs = (keys->find(lhs) != keys->end()) ? keys->operator[](lhs) : 0;
        const auto timestamp_rhs = (keys->find(rhs) != keys->end()) ? keys->operator[](rhs) : 0;
nlohmann commented 7 years ago

Thanks for reporting! The test suite is definitely not good enough - otherwise I should have noticed earlier!