This PR makes possible to cross-compile Windows build with --enable-werror --enable-suppress-external-warnings.
Some problems are fixed, others are silenced.
Also --enable-werror is enabled for Cirrus CI Windows build (the last one on Cirrus CI without --enable-werror).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
147@@ -148,7 +148,10 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
148 #ifdef __GLIBCXX__
149
150 // reference: https://github.com/gcc-mirror/gcc/blob/gcc-7_3_0-release/libstdc%2B%2B-v3/include/std/fstream#L270
151-
152+#if defined(__GNUC__) && !defined(__clang__)
153+#pragma GCC diagnostic push
154+#pragma GCC diagnostic ignored "-Wswitch"
155+#endif
156 static std::string openmodeToStr(std::ios_base::openmode mode)
-Wswitch for any GCC compiler (even latest on Linux). Shouldn’t we restrict that somehow? Maybe check if WIN32 is defined or for a specific GCC version?
#ifdef WIN32, sorry for the noise.
396+
397+ dnl -Wreturn-type is broken in GCC for MinGW-w64.
398+ dnl https://sourceforge.net/p/mingw-w64/bugs/306/
399+ AX_CHECK_COMPILE_FLAG([-Werror=return-type], [ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"], [], [$CXXFLAG_WERROR],
400+ [AC_LANG_SOURCE([[#include <cassert>
401+ int f(){ assert(false); }]])])
This is good and will work fine.
I am just curious, does that mingw bug 306 manifest if assert(false) is replaced with abort()?
I’m a bit divided on whether building on every platform/compiler combination ‘with -Werror’ is actually a goal of the project. I mean, I think this is impossible, and will result in a lot of warning fixing for the sake of warning fixing.
In general Werror is a curse.
ACK 06ff4756
Sometimes important warnings are emitted only on some platforms, so I think it is good to use -Werror on as most platforms as possible.
If keeping Windows builds warning-free turns out to be too big burden then the NO_WERROR=1 can be added back.
I’m a bit divided on whether building on every platform/compiler combination ‘with -Werror’ is actually a goal of the project. I mean, I think this is impossible, and will result in a lot of warning fixing for the sake of warning fixing.
In general Werror is a curse.
Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.
Fair enough.
The goal of this PR (combined with #20544) is to prevent changes that introduce new warnings.
Anyway, feel free to close this.
🕵️ @practicalswift has been requested to review this pull request as specified in the REVIEWERS file.
Updated 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464 -> 926f6d82a31b2d2c82b830b2b672fa38890b8590 (pr20586.01 -> pr20586.02, diff). @MarcoFalke
Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.
Only one-line change in fs.cpp now.
151+// reference: https://github.com/gcc-mirror/gcc/blob/releases%2Fgcc-7.3.0/libstdc%2B%2B-v3/include/std/fstream#L270
152
153 static std::string openmodeToStr(std::ios_base::openmode mode)
154 {
155- switch (mode & ~std::ios_base::ate) {
156+ switch (static_cast<int>(mode & ~std::ios_base::ate)) { // casting is required to suppress -Wswitch warnings
std::ios_base::ate and friends are defined as unsigned int on my platform. What is the warning that this tries to fix?
std::ios_base::ateand friends are defined asunsigned inton my platform.
What is the warning that this tries to fix?
https://gcc.gnu.org/onlinedocs/gcc-9.3.0/gcc/Warning-Options.html#Warning-Options:
-Wswitch…caselabels outside the enumeration range also provoke warnings when this option is used (even if there is adefaultlabel).
Ok, we are fine because if some of the case values is out of the range of int, then we would get a warning:
0void f(unsigned int x)
1{
2 const unsigned int y = 3000000000U;
3
4 switch (static_cast<int>(x)) {
5 case y:
6 std::cout << "foo\n";
7 break;
8 }
9}
0error: case value evaluates to 3000000000, which cannot be narrowed to type 'int' [-Wc++11-narrowing]
from clang 12 with -Wall
NACK, the standard doesn’t guarantee this fits in an int…
Can you use a pragma to disable the warning here?
Reverted back 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464 <- 926f6d82a31b2d2c82b830b2b672fa38890b8590 (pr20586.01 <- pr20586.02) as was requested by @luke-jr.
cr ACK 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464: patch looks correct
FWIW, having this in master would have prevented the issue #21355 pre-merge :)
147@@ -148,7 +148,10 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
148 #ifdef __GLIBCXX__
149
150 // reference: https://github.com/gcc-mirror/gcc/blob/gcc-7_3_0-release/libstdc%2B%2B-v3/include/std/fstream#L270
151-
152+#if defined(__GNUC__) && !defined(__clang__)
153+#pragma GCC diagnostic push
154+#pragma GCC diagnostic ignored "-Wswitch"
Is this commit " Fix Windows build with –enable-werror on Ubuntu Bionic (gcc 7.5) " still needed?
We no longer use mingw on bionic (only focal)
@MarcoFalke It is still required to prevent -Werror=switch warnings on Focal.
Therefore, technically, no changes are required in the patch.
Updated 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464 -> b367745cfe19f6de3f44b3adc90fa08e36e44bb6 (pr20586.01 -> pr20586.03):
ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6
Tested on Linux cross-compiling to Windows passing --enable-werror to configure
Master:
0script/standard.cpp:215:26: error: control reaches end of non-void function [-Werror=return-type]
1 215 | std::vector<valtype> vSolutions;
PR:
Builds fine and built binaries work fine on Windows 10
Do you mind having another look at this PR?
153@@ -154,7 +154,10 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
154 #ifdef __GLIBCXX__
155
156 // reference: https://github.com/gcc-mirror/gcc/blob/gcc-7_3_0-release/libstdc%2B%2B-v3/include/std/fstream#L270
157-
158+#if defined(__GNUC__) && !defined(__clang__)
159+#pragma GCC diagnostic push
160+#pragma GCC diagnostic ignored "-Wswitch"