Build everything as C++

[Rangi42]

The long-term goal here is to gradually convert the codebase to idiomatic C++. For example, we could get rid of the many reimplementations of dynamic arrays and just use std::vector.

This PR just makes the minimal changes for it to build. That includes:

• Renaming .c to .cpp and .h to .hpp.
• Casting the return values of malloc and realloc.
• Arranging designated array initializers (C++20 only) in numeric order, since gcc otherwise complains "non-trivial designated initializers not supported".
• Not using -pedantic, since we still have C-only nested anonymous structs and flexible array members, which are less trivial to replace.
• Use [[noreturn]] instead of _Noreturn.
• Use {} instead of {0}.
• Don't skip over a variable declaration with goto; move variables into a block or before the goto.
• Move nested named struct and enum definitions to the top-level scope.
• Enums aren't integers, which requires some more casting and + 1 instead of ++.

[ISSOtm]

Those CI errors are certainly confusing. Why is DLL list corrupted somehow?

As for __PRETTY_FUNCTION__ being a GNU extension, well, tough luck. I don't know if there is a MSVC equivalent.

[Rangi42]

__func__ would suffice for MSVC. Edit: Oh, it also has __FUNCSIG__.

[Rangi42]

Those CI errors are certainly confusing. Why is DLL list corrupted somehow?

You mean how the two 32-bit windows-xtesting CI runs give lots of cannot open shared object file: No such file or directory errors during the rgbfix tests? I don't know.

[Rangi42]

Blocked by choosing a solution to #1002.

[ISSOtm]

Unblocked!

[Rangi42]

Looks like Visual Studio doesn't support designated array initializers in C++, period; CI fails with a lot of "syntax error: unexpected token '=', expected '{'".

Edit: "This feature has been implemented in MSVC under /std:c++latest since VS 2019 16.1." And the windows-2019 CI image uses 16.11.34114.132. Why is this failing?

If nothing else, we can replace [index] = value, with AT(index) value, and in platform.hpp have #define AT(index) [index] = just for non-MSVC compilers.

[ISSOtm]

"This feature has been implemented in MSVC under /std:c++latest since VS 2019 16.1." And the windows-2019 CI image uses 16.11.34114.132. Why is this failing?

Are we passing that flag? Looks like CMake is generating /std:c++20 instead, but it seems that that flag should be sufficient starting with MSVC 16.11, which windows-2019 ships?

[Rangi42]

@ISSOtm Even passing /std:c++latest fails with MSVC. I can either stop using designated initializers (and thus stick with C++17), or use a platform.h macro to hide them from MSVC (so [index] = ... would become AT(index) ...).

[ISSOtm]

Aha, got it! C++20 added designated initialisers for structs, but not for arrays!

If nothing else, we can replace [index] = value, with AT(index) value, and in platform.hpp have #define AT(index) [index] = just for non-MSVC compilers.

Rather, the macro should only be defined for __GNUC__ compilers, since it's a non-standard extension.

[Rangi42]

The 32-bit windows-xtesting CI builds fail (but not the 64-bit ones). When it's running the rgbfix test script, it keeps complaining:

C:/Users/runneradmin/AppData/Local/Temp/tmp.GYsYzP8LO4/rgbfix.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory

This means the test cases aren't getting rgbfixed and so we get a lot of spurious .bin diff output.

Looking into this as an issue with the rgbds-canary-mingw-win32 artifact. Could there be a DLL it isn't copying into bins?

Edit: Putting back C-specific directives in CMakeLists.txt didn't help.

[ISSOtm]

I have one background concern with this PR: it's a big change, it carries a lot of risk (currently, the best example is the miscompilation under MinGW for 32-bit), and it may annoy downstream packagers.

But that doesn't mean we should throw any of this work away. Instead, I want to consider whether switching to C++ would enable any of the changes we have scheduled for 0.7.0—and I believe "enables feasibility for the launch window" counts as enabling.

If it does, then let's keep going ahead. But if it doesn't, I'd like to consider the possibility of buffering this PR until right after the 0.7.0 release.

The rationale for my being chilly here, is that if we're going to move to Rust in one of the next few releases, we may want to play nice with packagers, and avoid causing them extra work several times over.

Though maybe nont of that is a concern if the duration between 0.7.0 and the first Rust release ends up being similar to between 0.6.1 and 0.7.0 😆

[Rangi42]

@ISSOtm I'm more optimistic about making this switch before 0.7.0 releases. We already required a C++ compiler in the build for rgbgfx, so packagers have accounted for that. And as soon as we do convert to C++ I plan to try fixing some memory leaks and string length limits.

That said, I specifically set the 0.7.0 milestone tasks to be ones we can do quickly (and mostly have), without needing C++ to make them easier to implement.

The MinGW 32-bit issue is weird. I'll try and find the root cause and see if it was some one-off mistake, or a case of something about C++ compilation that really demonstrates a risk for other untested packaging environments.

[Rangi42]

@ISSOtm I fixed the MinGW 32-bit build error. rgbfix.exe was requiring some DLLs that we explicitly excluded from copying in the 32-bit version, one of which (libgcc_s_dw2-1.dll) doesn't even exist for the 32-bit CI run. (I tried finding it, it only exists as /usr/lib/gcc/i686-w64-mingw32/10-win32/libgcc_s_dw2-1.dll.) The accepted solution is apparently to statically link libgcc for 32-bit (see libusb/libusb#1049 and msys2/MINGW-packages#2600).

I hope this won't prevent us merging the C++ PR. It was basically the same debugging difficulty as #1083 had, made annoying because of how you have to wait for GitHub Actions and read its logs in debugging steps.

Note that -static-libgcc doesn't bloat the .exe files; the 32-bit ones in rgbds-canary-mingw-win32 are actually smaller than rgbds-canary-mingw-win64. Probably -flto helped, since libgcc_s_dw2-1.dll itself is 833 KB.

[ISSOtm]

We already required a C++ compiler in the build for rgbgfx, so packagers have accounted for that.

Okay, that's fair then.

[ISSOtm]

@ISSOtm I fixed the MinGW 32-bit build error. rgbfix.exe was requiring some DLLs that we explicitly excluded from copying in the 32-bit version, [...]

I don't understand why the error message printed that as a single question mark, but I'm glad you figured it out somehow.

[Rangi42]

This PR is just for the minimal changes required to compile as C++. But once it's merged, I'll do some further cleanup, like new/delete instead of casting malloc; std::vector instead of realloc; and there's this itertools template for easily iterating over sequential enums:

// Usage example:
// for (enum SectionType type : EnumSeq(SECTTYPE_INVALID)) { ... }

template<typename T>
class EnumSeqIterator {
	T _value;

public:
	explicit EnumSeqIterator(T value) : _value(value) {}

	EnumSeqIterator &operator++() {
		_value = (T)(_value + 1);
		return *this;
	}

	auto operator*() const { return _value; }

	friend auto operator==(EnumSeqIterator const &lhs, EnumSeqIterator const &rhs) {
		return lhs._value == rhs._value;
	}

	friend auto operator!=(EnumSeqIterator const &lhs, EnumSeqIterator const &rhs) {
		return lhs._value != rhs._value;
	}
};

template<typename T>
class EnumSeq {
	T _limit;

public:
	explicit EnumSeq(T limit) : _limit(limit) {}

	EnumSeqIterator<T> begin() { return EnumSeqIterator((T)0); }
	EnumSeqIterator<T> end() { return EnumSeqIterator(_limit); }
};

[ISSOtm]

A few remaining things, but this is practically mergeable.

Resolved