Replace bool fullscreen parameter with bitset enum

[falbrechtskirchinger]

I've reduced #1602 to just the change from bool fullscreen to enum class DisplayMode (or whatever we decide to call it in the end).

This depends on Return-To-The-Roots/libutil#28 and I've temporarily substituted my fork of libutil for development and CI.

[Flamefire]

@Flow86 This requires raising the minimum Boost version to 1.71, see Return-To-The-Roots/libutil#28 (comment)

[falbrechtskirchinger]

@Flamefire How would you feel about adding -Wno-unknown-attribute nodiscard to cmake/EnableWarnings.cmake (assuming it works as expected). Modern clang-tidy is very aggressively insisting I use [[nodiscard]] everywhere and I'd very much like to do so. Even if you raise the Boost version, I'd still prefer that.

[Flow86]

hmm, we could try to raise the c++ version to 17 instead?

[falbrechtskirchinger]

hmm, we could try to raise the c++ version to 17 instead?

That would be great. I miss C++17 features all the time. Flamefire mentioned the Apple Crosscompiler can't do C++17.

(First search result is https://github.com/tpoechtrager/osxcross. Is that it? Seems to handle even C++20 just fine. So you may just need to update?)

[Flow86]

That would be great. I miss C++17 features all the time. Flamefire mentioned the Apple Crosscompiler can't do C++17.

(First search result is https://github.com/tpoechtrager/osxcross. Is that it? Seems to handle even C++20 just fine. So you may just need to update?)

the old version of the compiler didnt do it, but I updated the toolchain months ago already, so it should be fine afaik
we probably need to check if we need to increase the min boost version though, but nevertheless, that would be fine.

Feel free to try it out raising the default c++ version to 17

[Flamefire]

hmm, we could try to raise the c++ version to 17 instead?

I think this will make a couple MSVC versions unavailable and not sure how high the gain is: The C++14 MinGW compiler doesn't even support C++11-threads on Windows last time I tried.

[falbrechtskirchinger]

The C++14 MinGW compiler doesn't even support C++11-threads on Windows last time I tried.

dockerImages = [
    "windows.i686"    : "rttr/cross-compiler/mingw/mingw-w64-docker:master",
    "windows.x86_64"  : "rttr/cross-compiler/mingw/mingw-w64-docker:master",
    "linux.x86_64"    : "rttr/cross-compiler/linux/linux-amd64-docker:master",
    "apple.x86_64"    : "rttr/cross-compiler/apple/apple-docker:master"
]

When you're talking about MinGW, are you referring to these Docker images in Jenkinsfile? Are the Dockerfiles for these images publicly available? I.e, could I submit a PR somewhere if needed? Unfortunately, the custom Jenkins instance also means I can't verify MinGW on a fork. (I really hope you're not using that dreadfully broken, outdated, yet extremely popular MinGW-w64 GCC 8.1.0 version from SourceForge.)

AppVeyor and GHA are currently in progress. If that passes I can submit a PR to try out Jenkins as well.

[falbrechtskirchinger]

Well, as could have been predicted, clang-tidy complains about thousands of missing [[nodiscard]]s. AppVeyor's still going but all green so far.

It may simply not be worth it to keep modernize-use-nodiscard enabled.

[Flamefire]

When you're talking about MinGW, are you referring to these Docker images in Jenkinsfile?

Yes

It may simply not be worth it to keep modernize-use-nodiscard enabled.

Agreed. The root clang-tidy on purpose enables everything and selectively disables things to get the new good things automatically. So this wasn't enabled on purpose.

BTW: You'd need to enable C++17 especially in the submodules as you are using the C++17 feature. For testing this you can (temporarly!) pass -DCMAKE_CXX_STANDARD=17 to CMake on CI which will use that in all sub-libraries. For Jenkins this could be put in tools/ci/jenkins/build.sh
Again: Only temporarily.
Note: cxx_std_14 is a usage requirement, i.e. "use at least C++14" (won't pass any -std=c++xx if the compilers default is C++14 or higher) while -DCMAKE_CXX_STANDARD=17 is a command/instruction, i.e. "use exactly this and pass -std=c++17 if this isn't the compilers default"

[falbrechtskirchinger]

It may simply not be worth it to keep modernize-use-nodiscard enabled.

Agreed. The root clang-tidy on purpose enables everything and selectively disables things to get the new good things automatically. So this wasn't enabled on purpose.

It would still be nice to get reminders in my IDE. Supposedly I can use a user configuration for clangd with some If conditions to scope rules to particular projects.

BTW: You'd need to enable C++17 especially in the submodules as you are using the C++17 feature. For testing this you can (temporarly!) pass -DCMAKE_CXX_STANDARD=17 to CMake on CI which will use that in all sub-libraries.

That's certainly a simpler solution for testing. Thanks.

For Jenkins this could be put in tools/ci/jenkins/build.sh Again: Only temporarily.

Of course.

Note: cxx_std_14 is a usage requirement, i.e. "use at least C++14" (won't pass any -std=c++xx if the compilers default is C++14 or higher) while -DCMAKE_CXX_STANDARD=17 is a command/instruction, i.e. "use exactly this and pass -std=c++17 if this isn't the compilers default"

I'm aware. After all, this is why BOOST_ATTRIBUTE_NODISCARD is working now. GCC has been defaulting to at least C++17 for a few years. I assume Clang does as well.

[Flow86]

I think this will make a couple MSVC versions unavailable

anything which is older than MSVC 2019 shouldn't be used anyway, especially since you get an up to date community edition for private use for free

For Jenkins this could be put in tools/ci/jenkins/build.sh

this won't do anything, since its using the "secure" one from master branch only

We're using the mingw package from debian bullseye btw.

Afaik Mingw does not support native "c++11" threads yet, and won't be soon, but we're using the posix variant instead

[falbrechtskirchinger]

I've modified AppVeyor and GHA CI to force C++17 and have verified from the verbose output that /std:c++17 and -std=c++17 are passed to the compilers, respectively.
I'm waiting for all builds to finish and to see if there are more clang-tidy checks I may have to disable.

If everything goes well, I would submit PRs to change the following lines to cxx_std_17:

libendian/CMakeLists.txt:target_compile_features(endian_interface INTERFACE cxx_std_14)
liblobby/CMakeLists.txt:target_compile_features(lobby PUBLIC cxx_std_14)
libsiedler2/CMakeLists.txt:target_compile_features(siedler2 PUBLIC cxx_std_14)
libutil/libs/common/CMakeLists.txt:target_compile_features(s25util_common PUBLIC cxx_std_14)
libutil/libs/log/CMakeLists.txt:target_compile_features(s25util_log PUBLIC cxx_std_14)
libutil/libs/network/CMakeLists.txt:target_compile_features(s25util_network PUBLIC cxx_std_14)
mygettext/CMakeLists.txt:target_compile_features(mygettext PUBLIC cxx_std_14)
s25edit/SGE/CMakeLists.txt:target_compile_features(SGE PUBLIC cxx_std_14)
s25update/src/CMakeLists.txt:target_compile_features(s25update PUBLIC cxx_std_14)

@Flow86 Is that alright with you? Would you like to perform a MinGW build with -DCMAKE_CXX_STANDARD=17 manually? I only have MinGW 13.2.0 on my system. I don't believe testing with that version (compared to 8.0.0 on bullseye) is meaningful. I do however have GCC 8.5.0 which I will try.

Edit:
There're a few things clang-tidy identifies, that I'd also fix in the relevant submodule PRs:

• readability-redundant-declaration: Remove the out-of-class definitions of static constexpr members.
• modernize-unary-static-assert: Remove empty strings from static_asserts.

Edit 2:
Here's the complete list of failed clang-tidy checks:

    285 [modernize-unary-static-assert]
    226 [readability-redundant-declaration]
     40 [modernize-concat-nested-namespaces]
     10 [modernize-use-auto]
      2 [performance-for-range-copy]

Many occurrences are in headers and counted multiple times, so this seems quite manageable.

Commands for future reference

find libs/ -type f -exec sed -i -e 's/^namespace \(.*\) { namespace /namespace \1::/' -e 's%^}} // namespace%} // namespace%' '{}' \+

[Flow86]

CMAKE_CXX_COMPILE_FEATURES for the mingw 8.x compiler also contains cxx_std_20 already (since its a gcc-10)

I've built it locally with -DCMAKE_CXX_STANDARD=17 and it worked flawlessly (I also checked that -std=c++17 is correctly set... but I expected it to build though, since we're not using any c++17 features (yet))

[falbrechtskirchinger]

Well, is there any reason to not jump to cxx_std_20 directly then? I didn't know that the Debian package version didn't match the GCC version (although it makes more sense now, given that a GCC 8.0.0 release doesn't even exist).
Visual Studio 2019 has C++20 support AFAIK.

This doesn't mean we need to embrace C++20 features yet, especially since older GCC and Clang version lack support for many features, but the option is on the table, once older GCCs and Clangs drop off and we don't need to go through that dance of updating the submodules twice.

[Flow86]

we still want to support some older gccs (gcc-9?) so 17 should be fine, 20 not (yet)

Even though that perhaps ranges or so would be in good use, but lets go to 17 first to see if we get any trouble somewhere

[falbrechtskirchinger]

I did bump it up to 20 out of curiosity and at least Boost 1.69 has some issues. Maybe in a year or two.

[falbrechtskirchinger]

Turns out that a bitset is not the best choice to encode the display mode and other window flags after all. I've tried to force myself to use it and it shows:

enum class WindowFlags : unsigned
{
    DisplayModeWindowed = 0,
    DisplayModeFullscreen = 1 << 0,
    DisplayModeWindowedFullscreen = 1 << 1,
    DisplayModeMask = DisplayModeWindowed | DisplayModeFullscreen | DisplayModeWindowedFullscreen,

    Resizable = 1 << 4,

    Default = DisplayModeWindowed | Resizable
};
MAKE_BITSET_STRONG(WindowFlags);

A plain enum class is much simpler and avoids any invalid/nonsensical bit combinations:

enum class DisplayMode {
    Windowed,
	WindowedNonResizable,
    WindowedFullscreen,
    Fullscreen
};

The information loss that prompted me to pursue bitsets is not a concern after a closer look.