Rewrite of `CInifile`

[AMS21]

• Completely handwritten parser which parses the entire ini file in one go including conversions to lowercase and without any string copies
• Fixed all sorts of crashes, infinite loops etc. I could find for invalid ini files. Instead of crashing we now report an (hopefully) helpful error message
• More lenient parsing for #include or inheritance thus

[a]
  #include "other_file"

[b] : a

will now be parsed just fine.

• Renamed Items members first and second to name and value
• Added and improved const-correctness for the entire CInifile class
• Moved all function definitions to the source files
• General cleanup of xr_ini.h and xr_ini.cpp

[Xottab-DUTY]

Just out of the top of my head:

Do these cases work correct?

[sect]
param = "multiline
value"

[sect2]
param = "value" ; single line

[bad_sect]
param = "value  ; missing closing quote

; there should be permissive mode in which parser should interpret missing closing quote as single line value and parse until the end of line
; permissive mode should be enabled for Shadow of Chernobyl, but disabled for CS and COP

[AMS21]

The last part no and now I understand why. I couldn't get Shadow of Chernobyl to run and didn't test that.

The other ones work as expected.

[Xottab-DUTY]

SOC ini parser doesn't support multiline values at all, so it just always parse values until the end of the line. For our COP parser I just added a simple fix: if we have reached EOF and still didn't found a closing quote, rollback and parse till end of the line. This decision allows to use multiline values for SOC, but may have introduced yet unknown corner cases.

For sanity and simplicity I suggest to disable multiline functionality completely for SOC mode, and allow modders to enable it in openxray.ltx, instead of keeping that run-time workaround.

And here's the code snippet of the idea: bool multiline_enabled = pSettingsOpenXRay->read_if_exists<bool>("compatibility", "ltx_multiline_values", !ShadowOfChernobylMode);

[AMS21]

SOC ini parser doesn't support multiline values at all, so it just always parse values until the end of the line. For our COP parser I just added a simple fix: if we have reached EOF and still didn't found a closing quote, rollback and parse till end of the line. This decision allows to use multiline values for SOC, but may have introduced yet unknown corner cases.

For sanity and simplicity I suggest to disable multiline functionality completely for SOC mode, and allow modders to enable it in openxray.ltx, instead of keeping that run-time workaround.

And here's the code snippet of the idea: bool multiline_enabled = pSettingsOpenXRay->read_if_exists<bool>("compatibility", "ltx_multiline_values", !ShadowOfChernobylMode);

Alright thanks for clarifying that. I've fixed the problems and with multiline support we get the following output when dumping the ini file:

! Ini File[test.ini]: parsing failed. Unterminated multiline value
[sect]
param = "multiline
value"

[sect2]
param = "value"

and without multiline support:

~ Ini File[test.ini]: Multiline values are not supported assuming single line with missing closing quote. '"multiline"'
~ Ini File[test.ini]: Multiline values are not supported assuming single line with missing closing quote. '"value  ; missing closing quote"'
[bad_sect]
param = "value  ; missing closing quote"

[sect]
param = "multiline"
value"

[sect2]
param = "value"

[AMS21]

Going to run some more tests on the new code and then reopen the PR :)

[AMS21]

Alright testing found no more serious errors.

The initialization order is bit complicated and I'm not sure if it's entirely correct.

• We default to ltx_multiline_values_enabled to true
• We load openxray.ltx first which will always support multiline values.
• We determine game mode via Core.Params and then set ltx_multiline_values depending on the compatibility setting or otherwise default to !ShadowOfChernobylMode
• We then load the rest of the .ltx files

[Xottab-DUTY]

ShadowOfChernobylMode is controlled by openxray.ltx also, not only by command line key

[AMS21]

ShadowOfChernobylMode is controlled by openxray.ltx also, not only by command line key

Yes you are right. My explanation was wrong :sweat:

[Neloreck]

Little bit not related to PR itself, but are there easy ways to introduce unit tests for cpp codebase in general? Parsing of ini files and results checking with few sample files could be ideal starting point for the project

[AMS21]

Little bit not related to PR itself, but are there easy ways to introduce unit tests for cpp codebase in general? Parsing of ini files and results checking with few sample files could be ideal starting point for the project

Sure just add a testing framework like Catch2 or gtest write a simple test which could just be to test a single function and run them with something like ctest.

Both liked frameworks are very easy to setup and have good CMake integration

Edit: Theres also doctest which should also work fine and tries to be as small and fast as possible

[Xottab-DUTY]

Little bit not related to PR itself, but are there easy ways to introduce unit tests for cpp codebase in general? Parsing of ini files and results checking with few sample files could be ideal starting point for the project

Sure just add a testing framework like Catch2 or gtest write a simple test which could just be to test a single function and run them with something like ctest.

Both liked frameworks are very easy to setup and have good CMake integration

Edit: Theres also doctest which should also work fine and tries to be as small and fast as possible

I'd like to highlight https://github.com/doctest/doctest which claims to be the fastest testing frameworks and also very light on compile times.

[AMS21]

I'd like to highlight https://github.com/doctest/doctest which claims to be the fastest testing frameworks and also very light on compile times.

Yes although we have to keep in mind that doctest is currently not actively maintained. See this issue

But the library was and probably is very stable and probably the easiest to integrate since it comes with a single header version.

[Neloreck]

I know it is easy to just hang around and leave such comments as mine regarding adding tests, but I would suggest adding it as milestone (issue / task in project etc). It is almost one year since I started rewriting lua codebase with typescript and if I knew total complexity of the project I probably would not even start doing it. Fair to say that C++ part of openxray is even more complex and few times larger

Once I changed spacing/align in xml files and game stopped working Once I changed order of sections in one ini file and another one was broken as result I am still not sure whether it is all case sensitive since everything is lowercase and I just follow general pattern etc

My point is that it is hard to track such things in PR reviews and even more scary to change when we have few game modes/build types available

---

Once tests are added, easiest things to test could be related to different kind of utils, ini-xml parsers (especially DLTX variant in the future), net packets save-load process where order and type is crucial, planner and other things that do not interfere with global context

[AMS21]

I am still not sure whether it is all case sensitive since everything is lowercase and I just > follow general pattern etc

For ini files section names are case insensitive and item name are case sensitive. For the rest I'm not sure.

No I totally agree that adding tests is a great idea. I'm actually thinking about adding some basic unit tests for the 2 parsers I worked on now.