Converting OXCE to rapidyaml

[Yankes]

@Yankes
>this is wrong signature for move constructor.
Well, I made them const so that I could more easily determine what was being copied, and what was being moved. That allowed me to find a few places in code which I fixed. But yeah, move constructor with const doesn't make sense.

>your version had three problems:
Nice catch. I should mention that, if a sav file is missing ---, it's corrupted, because the header contains critical data.

>I had helper class `class RawData`
I should probably have made readFileRaw and getYamlSaveHeaderRaw return RawData instead of char*.

YamlRootNodeReader currently copies that data to ryml::Tree's buffer (arena), so there's no need to keep that data.
I could make a version that would use parse_in_place instead of parse_in_arena, but then data would have to outlive YamlRootNodeReader instance, which means it would have to keep the data. It's not worth the effort, because it only makes parsing maybe 1% faster (skips 1 memory allocation + copy).

Anyway, yeah, unique_ptr with RawData would probably be a better solution instead of YamlRootNodeReader calling free itself. Probably also safer, because if there's an exception, the free doesn't get called.

Should I change stuff to use unique_ptr + RawData?

> and what was being moved

is this constructor even called? I recall that overload resolution always avoid this signature and `const&` is always preferred over `const&&`.

> Should I change stuff to use unique_ptr + RawData?

Yes, but RawData is unique_ptr + size. This means when you use one, you use both. For now we can replace `char*, size_t` args by `RawData`.

>  because it only makes parsing maybe 1% faster

Ok, if we remove other allocations then it would rise to araund 4%? Probably too much changes for small profit. For now we can keep as is, maybe in future after we stabilize codebase we can improve things here (like script too).

[Meridian]

>3. compilation for ios in xcode
Based on those linker errors my best guess is that the rapidyaml cpp files aren't being compiled. Does a build log contain any rapidyaml hpp file compilation?
It could also be that the #include in Yaml.h (#include "../../libs/rapidyaml/ryml.hpp") is wrong, if the hpp files aren't at the right place.

Yeah, it will be something like that.
I remember now, I had similar issues when miniz library was added.
I solved it by copying miniz sources to "src" (from "../../libs"), adding the new ones to the project and modifying includes to refer to new ones.
Ugly, but worked.

I tried the same hacks now with rapidyaml, but unfortunately it didn't work... there's too many includes to modify, the thing simply doesn't see itself.
I really don't get it how such a simple thing as adding files to a project can be so FUCKING complicated and unintuitive as in Apple XCode.

We'll need someone who understands XCode to do this... I don't have the nerves for this.

[Meridian]

Last update for today.

iOS build is working (and game starts).

So, apparently, there are 2 kinds of folders in XCode projects.
Blue folders and yellow folders, I kid you not.
Blue is bad, yellow is good.

When adding folders via menu or via context menu, it creates blue folders without giving you any option.
So instead you need to use drag-and-drop, which gives you an option to create a blue or yellow folder.
Then just tell it to create a yellow folder and it will work.

Summary: Kids, don't do a career in IT. It will ruin your health, your sanity and your entire life.

[Yankes]

Summary: Kids, don't do a career in IT. It will ruin your health, your sanity and your entire life.

"Apple software" not even once!

[Delian]

OpenXcom web app when?

So while trying to convert functions to use RawData I noticed something strange. In FileMap.cpp:58 we're calling mz_free() instead of SDL_free. But mz_free() is simply calling free().

Won't this fail on some platforms the same way rapidyaml failed? Meridian, do you remember having any crashes with handling zip files on other platforms in the past?

[Yankes]

probably all this is finally `free` but if possible best would not mix them and keep symmetric between allocating and deallocating.

[Delian]

> and what was being moved
https://github.com/Delian0/OpenXcom/commit/a3088ba91a797d1f1d03252f88a926610d44efea and
https://github.com/Delian0/OpenXcom/commit/684e8e513bca4c50da82f16203a57aec29228627

>is this constructor even called? I recall that overload resolution always avoid this signature and `const&` is always preferred over `const&&`.
There is no "avoiding". const&& simply doesn't get called because it usually doesn't exist. So those calls get redirected to a copy constructor.

>For now we can replace `char*, size_t` args by `RawData`.
https://github.com/Delian0/OpenXcom/commit/af88e23bcc2bebd01a52ae19d7c62a486830734f

>if we remove other allocations then it would rise to araund 4%?
I dunno what allocations you're talking about. There's 1 allocation when yaml is copied to Tree's buffer. And there's 1 allocation (for node data) when I call _tree->reserve(yaml.len / 16); So during parsing there's 0 allocations lol (technically there are some for the newline data, for node locations in document)
But yeah, it's about 1% of the YamlRootNodeReader::Parse(). Or 0.15% of the whole startup process. You'll have to look elsewhere for performance gains hehe

[Yankes]

> I dunno what allocations you're talking about. There's 1 allocation when yaml is copied to Tree's buffer. And there's 1 allocation (for node data)
I count at least 3 `new` there

[Delian]

You're... not wrong. But all those new's together don't add 1ms, because YamlRootNodeReader constructors are rarely called. Which is funny because I put them there to make class is smaller, but that doesn't matter because the class is never copied/moved.

[Meridian]

Meridian, do you remember having any crashes with handling zip files on other platforms in the past?

Actually no, I don't remember any platform-specific issues with zip loader.
Stoddard tested quite extensively on win, linux and android before it was merged. And there were no macos or ios bugreports afaik.

[Delian]

Ok. Then the reason rapidyaml failed on some platforms was never because of malloc/free. It was other issues like compiler failing to apply NRVO, or failing to detect platform.

[Meridian]

@Meridian
Researching...
...it seems rapidyaml is trying to forward declare certain std types (like std::string) for some reason. But the macro logic isn't able to determine which standard library it is.

Edit:
"Android NDK uses LLVM's libc++, with support for _LIBCPP_ABI_NAMESPACE inline namespace. Older versions of the Android NDK supported GNU libstdc++, indicated by __GLIBCXX__ or __GLIBCPP__."

You shouldn't have gotten the "unknown standard library" error, because the conditions check for those specific macros. I don't get it...

Don't know if it helps, but I am attaching two build logs:
1/ successful build log of OXCE 7.15 with yaml-cpp
2/ failed build log of new version with rapidyaml

There's still a good chance I didn't modify cmake script correctly, but right now I don't know what else to try.

(I wanted to try also a newer Android NDK/SDK, but upgrading doesn't look simple.)

[Delian]

Just as a test try changing:
rapidyaml\c4\std\string_fwd.hpp:31
from
#error "unknown standard library"
to
#include <string>

and rapidyaml\c4\std\vector_fwd.hpp:35
from
#error "unknown standard library"
to
#include <vector>

[Meridian]

Yeah, that helped.

Compilation was successful and the game starts.

[Delian]

Well, that's annoying. It's the dreaded "custom library fix" that we were trying to avoid hehe.

It would probably be a good idea to make an issue about it on rapidyaml's git, but I'm not sure what to write in the issue. For a good report, I'd have to include info on the exact build environment, target environment, etc. And I'm not even sure if this is the correct fix, or it just coincidentally works (because our project already includes string/vector upstream of the build process).

The right solution would probably be to find out why, in an android build, _LIBCPP_ABI_NAMESPACE or __GLIBCXX__ or __GLIBCPP__ aren't being detected. If those aren't set, which macro or compiler variable should be checked for then?
I dunno. What should I do?