Make a header for VariantUtilityFunctions

[aaronfranke]

The motivation behind this PR is to allow writing unit tests for VariantUtilityFunctions. Without this PR, the only way to write a test would be to include the .cpp file, but this would result in linker warnings about duplicate symbols.

Aside from tests, this also allows using the utility functions from anywhere in the C++ code, like in a custom module.

[akien-mga]

LGTM.

[YuriSizov]

To be clear, we would still be including the cpp file in the codebase (like editor\editor_resource_preview.cpp does)? Otherwise inlining wouldn't work, right?

[aaronfranke]

@YuriSizov I think it's fine if we don't have inlining in the test cases, right?

[YuriSizov]

Yes, I'm talking about other parts of the engine that include variant utils directly, such as the aforementioned class.

[aaronfranke]

@YuriSizov You are correct, I did not see that before (EDIT: well, that's because it was added only recently #64628).

Note that we must remove the inline keyword for this to compile (unless we want to move the full definitions into the header). However the compiler may inline these methods anyway when compiling variant_utility.cpp even without the keyword since it has the definition and it's very small.

[akien-mga]

I think losing the inline behavior is problematic here, but this should likely be benchmarked to see what the compiler actually does.

EditorResourcePreview should definitely not include a .cpp, that's very hacky, so the function(s) it needs could be moved to a header while keeping the rest in the .cpp maybe? WDYT @lawnjelly?

Edit: EditorResourcePreview seems to include VariantUtilityFunctions for var_to_str and str_to_var. Maybe those two should be moved somewhere more publicly accessible.

[lawnjelly]

I would get @reduz to check to be honest, I can't say I'm familiar with the template-fu that goes on in variant_utility.cpp, and there's a possible danger of double definitions of these which could be bad. I think the PR is fine, but am not really familiar enough to say is ok.

The suggestion seems correct that using the header from EditorResourcePreview will be less likely to inline, but is that even performance sensitive? If not, no problem. 🤷‍♂️

[aaronfranke]

For some context, I want to include the utility functions in another place: unit tests. If the unit tests include the .cpp that may be an acceptable amount of hackiness, but we can only include the .cpp file in at most one place, when including the .cpp in both unit tests and EditorResourcePreview the linker will throw an error about duplicate definitions.

[reduz]

This is something that we wanted to do with @vnen so many of those are available to the VM to use as inlined opcodes, so thumbs up on that and PR may be good anyway.

That said, if you want to do unit tests, I think its not a good idea to access them yourself directly.

Simply call Variant::call_utility_function for the unit tests I guess.

[akien-mga]

reduz's comments sound like an approval of the header change, so let's go with this.

[akien-mga]

Thanks!