Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking temporary workaround: _to_variant for MSVC #100605

Open
Ivorforce opened this issue Dec 19, 2024 · 0 comments
Open

Tracking temporary workaround: _to_variant for MSVC #100605

Ivorforce opened this issue Dec 19, 2024 · 0 comments

Comments

@Ivorforce
Copy link
Contributor

Ivorforce commented Dec 19, 2024

Tested versions

Since 34fa0bf.

System information

Only applicable to MSVC.

Issue description

This issue is a call for someone using MSVC to reproduce an MSVC bug we encountered in the Godot codebase, and to create a Minimum Reproducible Project to report to the MSVC team. If you plan to take on this task, please comment on the issue so that we avoid accidentally spamming the team with multiple reports.

Commit 34fa0bf has introduced a temporary workaround for an MSVC bug. It was described in more detail in #100426 and in the relevant code (generating gdvirtual.gen.inc):

// MSVC WORKAROUND START
// FIXME The below helper functions are needed to work around an MSVC bug.
// They should be removed (by modifying core/object/make_virtuals.py) once the bug ceases to be triggered.
// The bug is triggered by the following code:
// `Variant(arg)`
// Through the introduction of the move constructor, MSVC forgets that `operator Variant()`
// is also a valid way to resolve this call. So for some argument types, it fails the call because
// it cannot convert to `Variant`.
// The function `_to_variant` helps the compiler select `.operator Variant()` for appropriate arguments using SFINAE.
template <typename T, typename = void>
struct has_variant_operator : std::false_type {};
template <typename T>
struct has_variant_operator<T, std::void_t<decltype(std::declval<T>().operator Variant())>> : std::true_type {};
// Function that is enabled if T has `.operator Variant()`.
template <typename T>
_ALWAYS_INLINE_ typename std::enable_if<has_variant_operator<T>::value, Variant>::type
_to_variant(T&& t) {
return std::forward<T>(t).operator Variant();
}
// Function that is enabled if T does not have `.operator Variant()`.
template <typename T>
_ALWAYS_INLINE_ typename std::enable_if<!has_variant_operator<T>::value, Variant>::type
_to_variant(T&& t) {
return Variant(std::forward<T>(t));
}
// MSVC WORKAROUND END

In short, we have decided to merge the workaround, and try to create an MRP to report to the MSVC developers.
Such an MRP does not exist yet.

Steps to reproduce

Here's what I know about the MSVC bug:

It is triggered by the Variant(arg1) etc. calls of e.g. GDVIRTUAL3_REQUIRED against e.g. GDExtensionPtr<X>. In this specific configuration, MSVC forgets about the .operator Variant() conversion offered by GDExtensionPtr and fails to resolve the call (see this workflow run), e.g.:

Error: .\servers/audio/audio_effect.h(43): error C2440: '<function-style-cast>': cannot convert from 'GDExtensionPtr<AudioFrame>' to 'Variant'
.\servers/audio/audio_effect.h(43): note: 'Variant::Variant': ambiguous call to overloaded function
.\core/variant/variant.h(825): note: could be 'Variant::Variant(const Variant &)'
.\core/variant/variant.h(826): note: or       'Variant::Variant(Variant &&)'
.\core/variant/variant.h(455): note: or       'Variant::Variant(bool)'
.\servers/audio/audio_effect.h(43): note: while trying to match the argument list '(GDExtensionPtr<AudioFrame>)'
scons: *** [platform\windows\display_server_windows.windows.template_release.x86_64.obj] Error 2

It should be possible to reproduce this bug reliably on Godot master by just removing the MSVC workaround, by replacing this line:

callsiargs += f"_to_variant(arg{i + 1})"

with

        callsiargs += f"Variant(arg{i + 1})"

If it is not reproducible, we should be able to remove the workaround as-is :^)

Minimal reproduction project (MRP)

N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants