CODEWITHSUNDEEP
c++
browser-bug
browser-bug

Reputation: 2091

Strange behaviour with this struct shallow copy?

I've defined the following data structure which has the intent of providing a get_next() method which returns the iterator pointing at the next required_configuration. If required_explorations is empty or has reached the end, it returns it pointing at the begin.

#include <iostream>
#include <map>
#include <string>
#include <unordered_map>

using configuration_model = std::unordered_map<std::string, std::string>;

struct doe_model {

    inline bool add_config(const std::string& config_id,
        const configuration_model& config,
        const int required_number_of_observations)
    {
        bool assignment_took_place = !required_explorations.insert_or_assign(config_id, config).second || !number_of_explorations.insert_or_assign(config_id, required_number_of_observations).second;
        next = required_explorations.end();
        return assignment_took_place;
    }

    inline void update_config(const std::string& config_id)
    {
        number_of_explorations.at(config_id)--;

        // remove the configuration in case we exausted all the explorations
        if (number_of_explorations.at(config_id) <= 0)
            remove_config(config_id);
    }

    inline void remove_config(const std::string& config_id)
    {
        required_explorations.erase(config_id);
        number_of_explorations.erase(config_id);
    }

    // this method returns the next configuration to explore
    // NOTE: the caller MUST check the pointer first.
    inline std::map<std::string, configuration_model>::iterator get_next()
    {
        // we may have an empty map or one with only a single configuration left
        if (required_explorations.empty() || next == required_explorations.end()) {
            std::cout << "Required explorations is empty or next at the end(), "
                         "returning the first pointer."
                      << std::endl;
            next = required_explorations.begin();
            return next;
        }

        next++;

        std::cout << "Returning a next pointer normally." << std::endl;
        return next;
    }

    // key is the configuration_id
    std::map<std::string, configuration_model> required_explorations;
    std::map<std::string, configuration_model>::iterator next;
    std::map<std::string, int> number_of_explorations;
};

I'm not that aware on the default shallow copy that it is generated for an assignment operator with this structure, but testing it I think the problem relies on the next iterator copy.

Running this returns 0 as expected (which is the first element).

int main()
{
    doe_model doe;
    doe.add_config("0", { { "threads", "29" } }, 4);
    doe.add_config("1", { { "threads", "23" } }, 4);
    doe.add_config("2", { { "threads", "20" } }, 4);
    doe.add_config("3", { { "threads", "21" } }, 4);
    doe.add_config("4", { { "threads", "22" } }, 4);

    auto it = doe.get_next();
    std::cout << it->first << "\t" << std::endl;
}

But running the following returns 4 which is the end.

int main()
{
    doe_model doe;
    doe.add_config("0", { { "threads", "29" } }, 4);
    doe.add_config("1", { { "threads", "23" } }, 4);
    doe.add_config("2", { { "threads", "20" } }, 4);
    doe.add_config("3", { { "threads", "21" } }, 4);
    doe.add_config("4", { { "threads", "22" } }, 4);

    doe_model new_doe = doe;

    auto it = new_doe.get_next();
    std::cout << it->first << "\t" << std::endl;
}

Can someone give me some insight of what's going on behind the scenes?

UPDATE here's a real example of doe_model usage. There are some data members and other function calls which I'm not going to talk about since they aren't really needed in order to understand how doe_model is intended to be used. When send_configuration is called it needs to pull a new configuration from the doe structure and therefor the get_next method is needed to achieve that task returning a new configuration (different from the previous and which still has number_of_explorations > 0).

  void send_configuration(const client_id_t &name)
  {
    if (num_configurations_sent_per_iteration <= num_configurations_per_iteration)
    {
      auto configuration = doe.get_next();
      if (configuration != doe.required_explorations.end())
      {
        remote->send_message(
            {MESSAGE_HEADER + "/" + app_id.app_name + "^" + app_id.version + "^" + app_id.block_name + "/" + name + "/explore",
             configuration_to_json(configuration->second)});


        doe.update_config(configuration->first);

        num_configurations_sent_per_iteration++;
      }
    }
  }

Upvotes: 1

Views: 64

Answers (1)

Caleth
Caleth

Reputation: 63039

The member next; is copied, still referring to the other instance's required_explorations.

When you call get_next(), the comparison next == required_explorations.end() has undefined behaviour, it is only defined for iterators to the same container1.

Aside: In the case where required_explorations is empty, returning required_explorations.begin() and dereferencing it also has undefined behaviour.

  1. If you are using C++14 or later, you can compare your iterator to a value-initialised iterator, which behaves like end for all containers. In C++11 you need a second iterator from the container to be safe.

Upvotes: 3

Related Questions