C#: Fix MSVC dotnet builds failing if running dev_mode

[Repiteo]

Added #else section to mono #ifdef checks in export_plugin scripts. This not being in place caused compilation to fail when enabling extended warnings & warnings as errors, as otherwise the latter sections would be detected as "unreachable code"

[raulsntos]

Thanks for contributing to the .NET module! I was planning to fix those ambiguous cref for a while now, but never quite got around to it, so thanks! I appreciate it.

Added #else section to mono #ifdef checks in export_plugin scripts. This not being in place caused compilation to fail when enabling extended warnings, as otherwise the latter sections would be detected as "unreachable code"

Does CI enable these extended warnings? And are these the only places that cause these warnings?

[Repiteo]

Does CI enable these extended warnings? And are these the only places that cause these warnings?

CI doesn't inherently enable the warnings, that's still done with warnings=extra; but once that's enabled then it will flag them as warns every time: warning C4702: unreachable code. Those three files are the only places where it makes a fuss

[raulsntos]

CI doesn't inherently enable the warnings, that's still done with warnings=extra; but once that's enabled then it will flag them as warns every time

OK, so you are saying you want to change CI to enable these warnings? If so, I would do so in the same PR where you fix these warnings. Otherwise, it doesn't seem like a very useful change, since there's no guarantee that other unreachable code won't be added in the future.

Has this been previously discussed in the dev chat? I feel like I may be missing some context. Sorry if I sound dismissive, I'm just trying to understand the motivation behind these changes.

In general, I don't like these #ifdef changes because these are meant to be temporary until we add support for mobile/web platforms. So, removing them later becomes a bit more annoying since you have to find the #else and #endif and they're specially far apart in the Android code. Whereas before it was one contiguous chunk of code that could easily be removed.

But the code looks correct, so if it solves a real issue then I'm fine with it. However, there's no linked issue, and I haven't seen users complain about this warning before, so that's why I was asking.

[Repiteo]

I'm surprised this wasn't brought up before, it's been preventing me from enabling that & warnings as errors for .net builds. It could be something unique on my end if it's not happening to others, but I wouldn't know where to ask about it. I'm aware of the Rocket Chat, but I don't know which channel would be best (maybe I'm blind but I don't see a C# channel?)

I didn't consider adjusting the current warning system because I'm frankly unsure how that is handled. My only real motivation for the #else change was to make dev_mode=yes functional

[raulsntos]

maybe I'm blind but I don't see a C# channel?

There isn't a specific channel for C#. We use the #scripting channel which used to be for all scripting languages (GDScript, C# and VisualScript), but now GDScript has its own dedicated channel. Since there isn't much activity, we decided a dedicated channel for C# wasn't needed (at least for now).

I wouldn't know where to ask about it

I think it's a good fit for the #buildsystem channel, since it's about building Godot.

My only real motivation for the #else change was to make dev_mode=yes functional

I always use dev_mode=yes and have no issues, but I use Linux and I think your issues are with MSVC?

[Repiteo]

Thanks for the heads-up on the appropriate channels! Joined the former and asked about this in the latter to see if anyone else has experienced the same

For now, I assume that's it's MSVC-specific? I have no clue about the minutia of builds, so I wouldn't have considered that a different build environment would pick up on warnings differently!

Regardless, I don't really see much of an issue with the #else implementation. Maybe it could be made clearer with something like #endif // MODULE_MONO_ENABLED

[Repiteo]

Mobile moment

[Repiteo]

Made adjustments to the #else checks by bookending the relevant parts of the code to the top and bottom of their respective function. This way it should remain obvious what parts will need to be removed once that functionality is working properly

Also did two more warn fixes: one in ScriptManagerBridge to fix a possible null reference & one in StringExtensions that changes the ToLower and ToUpper functions to their invariant equivalents

[Repiteo]

Tested a bit more & found one more case where this wrapper was needed: dotnet builds where dev_build is disabled. In the uwp equivalent file, there were #ifndef blocks that also left unreachable code. It was already appended to the top of the respective functions, so it was a simple matter of swapping the #endif for an #else and putting a tagged #endif at the bottom of the functions

I sincerely don't know how nobody else had these issues before; I wish I had something more concrete to point to in that respect

[raulsntos]

Changing ToLower and ToUpper is technically a breaking change, since it changes behavior from using the current culture to the invariant culture. Depending on where it's used it may be correct, but I feel it warrants a discussion. Therefore my recommendation would be to keep those changes as a separate PR if you don't want to increase the scope of the PR.

I appreciate the fixes but I think it'd be better to keep the changes focused on one thing instead of trying to solve multiple things at once, since that increases the scope of the PR and makes it harder to review. Specially when so many small unrelated things are changed at the same time, not all of the changes may be desired and it makes maintainers hesitant to approve. I think this is why #65532 ended up in limbo, I've been extracting the changes from that PR to smaller PRs.

[Repiteo]

That's a very good point! I did say early on I didn't want to alter code functionality, so it'd be best to keep true to that

[Repiteo]

Given I have another docstring-focused PR with #79239, I migrated the relevant changes over there instead. Now this PR is focused squarely on fixing the dev_mode MSVC issue

[m4gr3d]

The changes look good.

[raulsntos]

This PR is meant to fix issues with building Godot .NET using MSVC with dev_mode=yes. I don't use MSVC and wasn't aware of any issues, I think @RedworkDE uses Windows so maybe knows more about it.

[RedworkDE]

Can confirm that these warnings do happen and that this does fix them, tho I normally don't use dev_mode for my builds so I hadn't seen these before.

[YuriSizov]

Thanks!