Refactor region to use reference_wrapper

Author: wvpm

src/openvic-simulation/economy/BuildingType.cpp

@@ -89,12 +89,12 @@ bool BuildingType::can_be_built_in(
 	if (is_limited_to_one_per_state) {
 		State const* state = location.get_state();
 		if (state != nullptr) {
-			for (ProvinceInstance const* province_in_state : state->get_provinces()) {
-				if (province_in_state == nullptr || *province_in_state == location) {
+			for (ProvinceInstance const& province_in_state : state->get_provinces()) {
+				if (province_in_state == location) {
 					continue;
 				}
 
-				const building_level_t other_building_level = province_in_state->get_buildings()[province_building_index.value()].get_level();
+				const building_level_t other_building_level = province_in_state.get_buildings()[province_building_index.value()].get_level();
 				if (other_building_level > building_level_t(0)) {
 					return false;
 				}

src/openvic-simulation/economy/production/ResourceGatheringOperation.cpp

@@ -400,8 +400,7 @@ void ResourceGatheringOperation::pay_employees(memory::vector<fixed_point_t>& re
 				upper_limit
 			);
 
-			for (Pop* owner_pop_ptr : *owner_pops_cache_nullable) {
-				Pop& owner_pop = *owner_pop_ptr;
+			for (Pop& owner_pop : *owner_pops_cache_nullable) {
 				const fixed_point_t income_for_this_pop = std::max(
 					revenue_left * owner_share.mul_div<pop_sum_t>(owner_pop.get_size(), total_owner_count_in_state_cache),
 					fixed_point_t::epsilon //revenue > 0 is already checked, so rounding up

src/openvic-simulation/economy/production/ResourceGatheringOperation.hpp

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <functional>
+
 #include "openvic-simulation/economy/production/Employee.hpp"
 #include "openvic-simulation/types/fixed_point/FixedPoint.hpp"
 #include "openvic-simulation/types/IndexedFlatMap.hpp"
@@ -25,7 +27,7 @@ namespace OpenVic {
 		ProvinceInstance* location_ptr = nullptr;
 		pop_sum_t total_owner_count_in_state_cache = 0;
 		pop_sum_t total_worker_count_in_province_cache = 0;
-		memory::vector<Pop*> const* owner_pops_cache_nullable = nullptr;
+		memory::vector<std::reference_wrapper<Pop>> const* owner_pops_cache_nullable = nullptr;
 
 		ProductionType const* PROPERTY_RW(production_type_nullable);
 		fixed_point_t PROPERTY(revenue_yesterday);

src/openvic-simulation/map/MapDefinition.cpp

@@ -371,22 +371,26 @@ bool MapDefinition::set_water_province(std::string_view identifier) {
 		return false;
 	}
 
-	ProvinceDefinition* province = get_province_definition_by_identifier(identifier);
+	ProvinceDefinition* const province_ptr = get_province_definition_by_identifier(identifier);
 
-	if (province == nullptr) {
+	if (province_ptr == nullptr) {
 		spdlog::error_s("Unrecognised water province identifier: {}", identifier);
 		return false;
 	}
-	if (province->is_water()) {
+	ProvinceDefinition& province = *province_ptr;
+	if (province.is_water()) {
 		spdlog::warn_s("Province {} is already a water province!", identifier);
 		return true;
 	}
 	if (!water_provinces.add_province(province)) {
 		spdlog::error_s("Failed to add province {} to water province set!", identifier);
 		return false;
 	}
-	province->water = true;
-	path_map_sea.try_add_point(province->get_province_number(), path_map_land.get_point_position(province->get_province_number()));
+	province.water = true;
+	path_map_sea.try_add_point(
+		province.get_province_number(),
+		path_map_land.get_point_position(province.get_province_number())
+	);
 	return true;
 }
 
@@ -417,7 +421,11 @@ size_t MapDefinition::get_water_province_count() const {
 	return water_provinces.size();
 }
 
-bool MapDefinition::add_region(std::string_view identifier, memory::vector<ProvinceDefinition const*>&& provinces, colour_t colour) {
+bool MapDefinition::add_region(
+	std::string_view identifier,
+	memory::vector<std::reference_wrapper<const ProvinceDefinition>>&& provinces,
+	colour_t colour
+) {
 	if (identifier.empty()) {
 		spdlog::error_s("Invalid region identifier - empty!");
 		return false;
@@ -428,8 +436,7 @@ bool MapDefinition::add_region(std::string_view identifier, memory::vector<Provi
 	// mods like DoD + TGC use meta regions of water provinces
 	bool is_meta = provinces.empty();
 	size_t valid_provinces_count { provinces.size() };
-	for (ProvinceDefinition const* const province_definition_ptr : provinces) {
-		ProvinceDefinition const& province_definition = *province_definition_ptr;
+	for (ProvinceDefinition const& province_definition : provinces) {
 		if (OV_unlikely(province_definition.has_region())) {
 			spdlog::warn_s(
 				"Province {} is assigned to multiple regions, including {} and {}. First defined region wins.",
@@ -456,9 +463,9 @@ bool MapDefinition::add_region(std::string_view identifier, memory::vector<Provi
 		ret &= region.add_provinces(provinces);
 	} else {
 		region.reserve(valid_provinces_count);
-		for (ProvinceDefinition const* const province_definition_ptr : provinces) {
-			if (!province_definition_ptr->has_region()) {
-				region.add_province(province_definition_ptr);
+		for (ProvinceDefinition const& province_definition : provinces) {
+			if (!province_definition.has_region()) {
+				region.add_province(province_definition);
 			}
 		}
 	}
@@ -467,8 +474,8 @@ bool MapDefinition::add_region(std::string_view identifier, memory::vector<Provi
 	if (OV_unlikely(is_meta)) {
 		SPDLOG_INFO("Region {} is meta.", identifier);
 	} else {
-		for (ProvinceDefinition const* province_definition : region.get_provinces()) {
-			remove_province_definition_const(province_definition)->region = &region;
+		for (ProvinceDefinition const& province_definition : region.get_provinces()) {
+			get_mutable_province_definition(province_definition).region = &region;
 		}
 	}
 	return ret;
@@ -640,10 +647,10 @@ bool MapDefinition::load_region_file(ast::NodeCPtr root, std::span<const colour_
 	const bool ret = expect_dictionary_reserve_length(
 		regions,
 		[this, &colours](std::string_view region_identifier, ast::NodeCPtr region_node) -> bool {
-			memory::vector<ProvinceDefinition const*> provinces;
+			memory::vector<std::reference_wrapper<const ProvinceDefinition>> provinces;
 
 			bool ret = expect_list_reserve_length(
-				provinces, expect_province_definition_identifier(vector_callback_pointer(provinces))
+				provinces, expect_province_definition_identifier(vector_emplace_callback(provinces))
 			)(region_node);
 
 			ret &= add_region(region_identifier, std::move(provinces), colours[regions.size() % colours.size()]);
@@ -1124,10 +1131,10 @@ bool MapDefinition::load_climate_file(ModifierManager const& modifier_manager, a
 				ret &= expect_list_reserve_length(*cur_climate, expect_province_definition_identifier(
 					[cur_climate, &identifier](ProvinceDefinition& province) {
 						if (province.climate != cur_climate) {
-							cur_climate->add_province(&province);
+							cur_climate->add_province(province);
 							if (province.climate != nullptr) {
 								Climate* old_climate = const_cast<Climate*>(province.climate);
-								old_climate->remove_province(&province);
+								old_climate->remove_province(province);
 								spdlog::warn_s(
 									"Province with id {} found in multiple climates: {} and {}",
 									province, identifier, *old_climate
@@ -1168,13 +1175,13 @@ bool MapDefinition::load_continent_file(ModifierManager const& modifier_manager,
 			}
 
 			ModifierValue values;
-			memory::vector<ProvinceDefinition const*> prov_list;
+			memory::vector<std::reference_wrapper<const ProvinceDefinition>> prov_list;
 			bool ret = NodeTools::expect_dictionary_keys_and_default(
 				modifier_manager.expect_base_province_modifier(values),
 				"provinces", ONE_EXACTLY, expect_list_reserve_length(prov_list, expect_province_definition_identifier(
 					[&prov_list](ProvinceDefinition const& province) -> bool {
 						if (province.continent == nullptr) {
-							prov_list.emplace_back(&province);
+							prov_list.emplace_back(province);
 						} else {
 							spdlog::warn_s("Province {} found in multiple continents", province);
 						}
@@ -1194,8 +1201,8 @@ bool MapDefinition::load_continent_file(ModifierManager const& modifier_manager,
 			continent.add_provinces(prov_list);
 			continent.lock();
 
-			for (ProvinceDefinition const* prov : continent.get_provinces()) {
-				remove_province_definition_const(prov)->continent = &continent;
+			for (ProvinceDefinition const& prov : continent.get_provinces()) {
+				get_mutable_province_definition(prov).continent = &continent;
 			}
 
 			return ret;

src/openvic-simulation/map/MapDefinition.hpp

@@ -2,6 +2,7 @@
 
 #include <cstdint>
 #include <filesystem>
+#include <functional>
 #include <string_view>
 
 #include <openvic-dataloader/csv/LineObject.hpp>
@@ -114,20 +115,23 @@ namespace OpenVic {
 	private:
 		ProvinceDefinition* get_province_definition_at(ivec2_t pos);
 
-		/* This provides a safe way to remove the const qualifier of a ProvinceDefinition const*, via a non-const Map.
-		 * It uses a const_cast (the fastest/simplest solution), but this could also be done without it by looking up the
-		 * ProvinceDefinition* using the ProvinceDefinition const*'s index. Requiring a non-const Map ensures that this
-		 * function can only be used where the ProvinceDefinition* could already be accessed by other means, such as the
+		/* This provides a safe way to remove the const qualifier of a ProvinceDefinition const&, via a non-const Map.
+		 * It looks up the ProvinceDefinition& using the ProvinceDefinition const&'s index. Requiring a non-const Map ensures that this
+		 * function can only be used where the ProvinceDefinition& could already be accessed by other means, such as the
 		 * index method, preventing misleading code, or in the worst case undefined behaviour. */
-		constexpr ProvinceDefinition* remove_province_definition_const(ProvinceDefinition const* province) {
-			return const_cast<ProvinceDefinition*>(province);
+		constexpr ProvinceDefinition& get_mutable_province_definition(ProvinceDefinition const& province) {
+			return *province_definitions.get_item_by_index(province.index);
 		}
 
 	public:
 		ProvinceDefinition const* get_province_definition_at(ivec2_t pos) const;
 		bool set_max_provinces(ProvinceDefinition::index_t new_max_provinces);
 
-		bool add_region(std::string_view identifier, memory::vector<ProvinceDefinition const*>&& provinces, colour_t colour);
+		bool add_region(
+			std::string_view identifier,
+			memory::vector<std::reference_wrapper<const ProvinceDefinition>>&& provinces,
+			colour_t colour
+		);
 
 		bool load_province_definitions(std::span<const ovdl::csv::LineObject> lines);
 		/* Must be loaded after adjacencies so we know what provinces are coastal, and so can have a port */

src/openvic-simulation/map/ProvinceInstance.cpp

@@ -53,7 +53,7 @@ ModifierSum const& ProvinceInstance::get_owner_modifier_sum() const {
 	return owner->get_modifier_sum();
 }
 
-memory::vector<Pop*> const& ProvinceInstance::get_pops_cache_by_type(PopType const& key) const {
+memory::vector<std::reference_wrapper<Pop>> const& ProvinceInstance::get_pops_cache_by_type(PopType const& key) const {
 	return pops_cache_by_type.at(key);
 }
 
@@ -215,7 +215,7 @@ void ProvinceInstance::_update_pops(MilitaryDefines const& military_defines) {
 
 	has_unaccepted_pops = false;
 
-	for (memory::vector<Pop*>& pops_cache : pops_cache_by_type.get_values()) {
+	for (memory::vector<std::reference_wrapper<Pop>>& pops_cache : pops_cache_by_type.get_values()) {
 		pops_cache.clear();
 	}
 
@@ -227,7 +227,7 @@ void ProvinceInstance::_update_pops(MilitaryDefines const& military_defines) {
 		: is_owner_core() ? fixed_point_t::_1 : military_defines.get_pop_size_per_regiment_non_core_multiplier();
 
 	for (Pop& pop : pops) {
-		pops_cache_by_type.at(pop.get_type()).push_back(&pop);
+		pops_cache_by_type.at(pop.get_type()).push_back(pop);
 		pop.update_gamestate(military_defines, owner, pop_size_per_regiment_multiplier);
 		add_pops_aggregate(pop);
 		if (pop.get_culture_status() == Pop::culture_status_t::UNACCEPTED) {

src/openvic-simulation/map/ProvinceInstance.hpp

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <functional>
+
 #include <plf_colony.h>
 
 #include "openvic-simulation/core/portable/ForwardableSpan.hpp"
@@ -122,7 +124,7 @@ namespace OpenVic {
 		bool convert_rgo_worker_pops_to_equivalent(ProductionType const& production_type);
 		void initialise_rgo();
 
-		OV_IFLATMAP_PROPERTY(PopType, memory::vector<Pop*>, pops_cache_by_type);
+		OV_IFLATMAP_PROPERTY(PopType, memory::vector<std::reference_wrapper<Pop>>, pops_cache_by_type);
 	public:
 		ProvinceDefinition const& province_definition;
 

src/openvic-simulation/map/Region.cpp

@@ -5,19 +5,14 @@
 
 using namespace OpenVic;
 
-bool ProvinceSet::add_province(ProvinceDefinition const* province) {
+bool ProvinceSet::add_province(ProvinceDefinition const& province) {
 	if (locked) {
 		spdlog::error_s("Cannot add province to province set - locked!");
 		return false;
 	}
 
-	if (province == nullptr) {
-		spdlog::error_s("Cannot add province to province set - null province!");
-		return false;
-	}
-
 	if (contains_province(province)) {
-		spdlog::warn_s("Cannot add province {} to province set - already in the set!", *province);
+		spdlog::warn_s("Cannot add province {} to province set - already in the set!", province);
 		return false;
 	}
 
@@ -26,20 +21,15 @@ bool ProvinceSet::add_province(ProvinceDefinition const* province) {
 	return true;
 }
 
-bool ProvinceSet::remove_province(ProvinceDefinition const* province) {
+bool ProvinceSet::remove_province(ProvinceDefinition const& province) {
 	if (locked) {
 		spdlog::error_s("Cannot remove province from province set - locked!");
 		return false;
 	}
 
-	if (province == nullptr) {
-		spdlog::error_s("Cannot remove province from province set - null province!");
-		return false;
-	}
-
 	const decltype(provinces)::const_iterator it = std::find(provinces.begin(), provinces.end(), province);
 	if (it == provinces.end()) {
-		spdlog::warn_s("Cannot remove province {} from province set - not in the set!", *province);
+		spdlog::warn_s("Cannot remove province {} from province set - not in the set!", province);
 		return false;
 	}
 
@@ -92,8 +82,8 @@ void ProvinceSet::reserve_more(size_t size) {
 	OpenVic::reserve_more(*this, size);
 }
 
-bool ProvinceSet::contains_province(ProvinceDefinition const* province) const {
-	return province != nullptr && std::find(provinces.begin(), provinces.end(), province) != provinces.end();
+bool ProvinceSet::contains_province(ProvinceDefinition const& province) const {
+	return std::find(provinces.begin(), provinces.end(), province) != provinces.end();
 }
 
 ProvinceSetModifier::ProvinceSetModifier(std::string_view new_identifier, ModifierValue&& new_values, modifier_type_t new_type)

src/openvic-simulation/map/Region.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <functional>
 #include <ranges>
 #include <string_view>
 
@@ -12,29 +13,28 @@ namespace OpenVic {
 
 	struct ProvinceSet {
 	private:
-		memory::vector<ProvinceDefinition const*> SPAN_PROPERTY(provinces);
+		memory::vector<std::reference_wrapper<const ProvinceDefinition>> SPAN_PROPERTY(provinces);
 		bool locked = false;
 
 	public:
 		/* Returns true if the province is successfully added, false if not (including if it's already in the set). */
-		bool add_province(ProvinceDefinition const* province);
+		bool add_province(ProvinceDefinition const& province);
 
 		template<std::ranges::sized_range Container>
-		requires std::convertible_to<std::ranges::range_value_t<Container>, ProvinceDefinition const*>
+		requires std::convertible_to<std::ranges::range_value_t<Container>, std::reference_wrapper<const ProvinceDefinition>>
 		bool add_provinces(Container const& new_provinces) {
 			reserve_more(new_provinces.size());
 
 			bool ret = true;
-
-			for (ProvinceDefinition const* province : new_provinces) {
+			for (ProvinceDefinition const& province : new_provinces) {
 				ret &= add_province(province);
 			}
 
 			return ret;
 		}
 
 		/* Returns true if the province is successfully removed, false if not (including if it's not in the set). */
-		bool remove_province(ProvinceDefinition const* province);
+		bool remove_province(ProvinceDefinition const& province);
 		void lock(bool log = false);
 		bool is_locked() const;
 		void reset();
@@ -43,7 +43,7 @@ namespace OpenVic {
 		size_t capacity() const;
 		void reserve(size_t size);
 		void reserve_more(size_t size);
-		bool contains_province(ProvinceDefinition const* province) const;
+		bool contains_province(ProvinceDefinition const& province) const;
 	};
 
 	struct ProvinceSetModifier : Modifier, ProvinceSet {