Move country parsing functionality to CountryParser #640

Spartan322 · 2025-11-12T19:38:53Z

Expose get_##singular##_registry in IdentifierRegistr
Expose setters for CountryDefinition unit colours
Expose non-const accessors for country_definitions

wvpm · 2025-11-12T19:47:37Z

src/openvic-simulation/country/CountryDefinition.hpp

I think it's better to move the CountryDefinitionManager out.
Either move it to a separate file or split it between the DefinitionManager and the CountryParser entirely (my preference).

src/openvic-simulation/country/CountryDefinition.hpp

src/openvic-simulation/dataloader/Dataloader.cpp

wvpm · 2025-11-12T19:54:45Z

src/openvic-simulation/dataloader/Dataloader.cpp

+		if (country_parser.load_country_list() != Error::OK) {
+			spdlog::critical_s("Failed to load country list!");
+			ret = false;
+		} else if ( //


This if-else-if chain obscures the happy flow.
I think making it one method on the country_parser and returning false when a step fails would be better.

So here something like

ret = country_parser.load_all(...);

And in the country_parser

bool CountryParser::load_all(...) { if (!load_country_list()) { //log return false; } if (!load_countries_from(...)) { //log return false; } ... }

It might even be better to move the logging into the various methods.

I don't agree with creating auxiliary functions in classes just for the sake of semantic sugar. If there is to be such, it should be contained entirely to the Dataloader. Our classes should be explicit.

Error _load_countries(DefinitionManager& definition_manager);
Should be part of the CountryParser, not the Dataloader.
The CountryParser has to load stuff in order, this order should be part of the CountryParser itself, not the Dataloader.

wvpm · 2025-11-12T19:55:37Z

src/openvic-simulation/dataloader/parse/CountryParser.cpp

+
+	bool is_dynamic = false;
+
+	const bool ret =


avoid lines ending on operators like const bool ret =

There is literally no way to solve this with clang-format, I don't like having preference-based style nitpicks.

See comment below, that is the way to fix it.
const bool ret = expect_dictionary_reserve_length( (with the rest on lines below it)

wvpm · 2025-11-12T19:56:21Z

src/openvic-simulation/dataloader/parse/CountryParser.cpp

+	bool is_dynamic = false;
+
+	const bool ret =
+		expect_dictionary_reserve_length(tags, [this, &is_dynamic](std::string_view key, ast::NodeCPtr value) -> bool {


You could do something like

const bool ret = expect_dictionary_reserve_length( tags, [this, &is_dynamic](std::string_view key, ast::NodeCPtr value) -> bool {

I don't agree, and indentation is important even in that case, that aside that would wreck the indentation of every other line, this is the more preferable option and one that clang-format can actually handle, the alternative is way worse and liable to break clang-format completely.

wvpm · 2025-11-12T19:58:41Z

src/openvic-simulation/dataloader/parse/CountryParser.cpp

+
+	for (TagData const& data : tags) {
+		const fs::path path = dataloader.lookup_file(StringUtils::append_string_views(common_folder, data.path));
+		if (Error load_err = load_country(data.tag, data.is_dynamic, Dataloader::parse_defines(path).get_file_node(), manager);


This is way too long for an if statement.
Also it ends with ; which is counterintuitive.
Better to do:

Error load_error = load_country( arg0, arg1, arg2, ... ); if (load_error != Error::OK) { ... }

Variable declarations should always apply to the narrowest scope, this is in compliance and expected with the C++ guidelines.

The whole thing would be much cleaner if you had something like a |= operator for your error type, with logic if right-hand-side != Error::OK : this = right-hand-side, so you could just write err |= load_country(...);

I feel like &= would be the expected operator for that since 1 & 0 == 0, (despite the fact Error::OK is 0, but it doesn't convert to bool so that doesn't really matter) it would be expected behavior that the first error value is retained and the rest are lost since subsequent errors in a lot of cases are likely a consequence of the first one. Even if they aren't it seems like it would make the most sense to report the first error since it at least follows the usual manner you'd solve problems with a debugger.

Historically, errors have often been represented as a bit set, where each bit position can be toggled on to signal that a particular error condition has been hit. Thus for those implementations, |= accumulates the union of the different error bits, and hence error conditions, that have been observed. That is why I pick |= as a reasonable default choice for my implementations, because it "makes sense" to someone who is familiar with that convention. If you aren't used to handling errors in that way, then obviously it doesn't matter one way or the other.

An if statement must contain only a condition.
Adding code inside of it and terminating it with ; is code shittification.
Sure, the c++ compiler can compile it... For humans, it just makes the code less readable.

As for "Variable declarations should always apply to the narrowest scope, this is in compliance and expected with the C++ guidelines." /care

You can easily reassign the load_error variable or simply make different ones like load_country_error. Alternatively, you could add a scope via braces.

if(var x; x) { ... } is generally recommended if you are using C++17 or later. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-cond The rationale is that it minimizes the scope where the value is live, which makes the code easier to reason about locally

I proposed #642 as a means of performing a bitset of the errors, for this case its not all that helpful since right now we only throw OK or FAILED, but the aim of the errors should be to support reporting all relevant errors, the ErrorSet makes it a lot easier to do that.

wvpm · 2025-11-12T19:59:24Z

src/openvic-simulation/dataloader/parse/CountryParser.cpp

+}
+
+OV_SPEED_INLINE NodeTools::NodeCallback auto CountryParser::_expect_country_party( //
+	IdentifierRegistry<CountryParty>& r_parties_registry


Why is it prefixed r_?

To publicly designate to the caller in the interface that its purely a return value.

This is the only case I've seen this prefix being used and we use the concept more often.
If you really want this, you should change the other locations as well.
Imo, just drop it. It's not a commonly known pattern and may only confuse others.

Its actually quite common in C++, (and even more common in C) a large number of libraries use that style or something akin to it since there's no way to force the caller's awareness to a reference/pointer being a purely a return value, signifying to the caller that they shouldn't treat it as an input. Regardless I'll move towards changing it throughout the codebase later.

wvpm · 2025-11-12T20:02:32Z

src/openvic-simulation/dataloader/parse/CountryParser.cpp

+						return party_policy_registry->expect_item_identifier(
+							[&policy, &party_policy_group](PartyPolicy const& party_policy) -> bool {
+								if (&party_policy.get_issue_group() == &party_policy_group) {
+									policy = &party_policy;


While this is valid, passing a reference to a pointer to a lambda that's passed to the expect_item_identifier method doesn't help with understanding the code.

It would be much clearer if we did policies.at(party_policy_group) = party_policy or policies.set(party_policy_group, party_policy) here.

wvpm · 2025-11-12T20:04:47Z

src/openvic-simulation/dataloader/parse/CountryParser.cpp

+			"start_date", ONE_EXACTLY, expect_date(assign_variable_callback(start_date)),
+			"end_date", ONE_EXACTLY, expect_date(assign_variable_callback(end_date)),
+			"ideology", ONE_EXACTLY,
+				ideology_registry->expect_item_identifier(assign_variable_callback_pointer(ideology), false)


Outside of scope for this PR, but couldn't assign_variable_callback_pointer be shortened to assign_pointer?
If you work with the data loading code, you know everything is a callback.

wvpm · 2025-11-12T20:06:26Z

src/openvic-simulation/country/CountryDefinition.hpp

-		colour_t PROPERTY(tertiary_unit_colour);
-		// Unit colours not const due to being added after construction
-
+		colour_t PROPERTY_RW(primary_unit_colour);


Why use property here?
Wouldn't exposing the fields work just fine?

No because PROPERTY only generate a const getter, (and even then colour returns by value anyway) the unit colours need to be assigned.

wvpm

Overall a step in a better direction.

Expose `get_##singular##_registry` in IdentifierRegistry Expose setters for CountryDefinition unit colours Expose non-const accessors for country_definitions

wvpm · 2025-11-13T08:28:30Z

src/openvic-simulation/dataloader/parse/CountryParser.hpp

+
+namespace OpenVic {
+	class CountryParser {
+		IdentifierRegistry<GovernmentType> const* government_type_registry;


Suggested change

IdentifierRegistry<GovernmentType> const* government_type_registry;

IdentifierRegistry<GovernmentType> const& government_type_registry;

As well as for the others.
You force const& via the constructor, might as well store it.

The issue in this case is there is no reason to prohibit generating of move and assignment operations, the CountryParser doesn't benefit from being copy only.

Spartan322 added enhancement New feature or request topic:codestyle topic:dataloader labels Nov 12, 2025

Spartan322 force-pushed the move/country-parsing branch from 36182e7 to 25e5b32 Compare November 12, 2025 19:40

wvpm reviewed Nov 12, 2025

View reviewed changes

src/openvic-simulation/country/CountryDefinition.hpp Show resolved Hide resolved

wvpm reviewed Nov 12, 2025

View reviewed changes

src/openvic-simulation/dataloader/Dataloader.cpp Outdated Show resolved Hide resolved

wvpm reviewed Nov 12, 2025

View reviewed changes

Move country parsing functionality to CountryParser

a3db7e2

Expose `get_##singular##_registry` in IdentifierRegistry Expose setters for CountryDefinition unit colours Expose non-const accessors for country_definitions

Spartan322 force-pushed the move/country-parsing branch from 25e5b32 to a3db7e2 Compare November 12, 2025 20:30

wvpm reviewed Nov 13, 2025

View reviewed changes

	IdentifierRegistry<GovernmentType> const* government_type_registry;
	IdentifierRegistry<GovernmentType> const& government_type_registry;

Move country parsing functionality to CountryParser #640

Are you sure you want to change the base?

Move country parsing functionality to CountryParser #640

Uh oh!

Conversation

Spartan322 commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spartan322 Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spartan322 Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spartan322 Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvpm Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spartan322 Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvpm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Spartan322 commented Nov 12, 2025 •

edited

Loading

Spartan322 Nov 12, 2025 •

edited

Loading

Spartan322 Nov 12, 2025 •

edited

Loading

Spartan322 Nov 13, 2025 •

edited

Loading

wvpm Nov 12, 2025 •

edited

Loading

Spartan322 Nov 12, 2025 •

edited

Loading