-W*-whitespace
#32482
GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable -Wleading-whitespace and -Wtrailing-whitespace.
We currently get PRs that are opened with various whitespace, i.e #33822, so turning that into compile-time failure where possible, seems useful, to avoid a CI roundtrip.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32482.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Possible typos and grammar issues:
No other added lines contain typographic or grammatical errors that make the English invalid or incomprehensible.
drahtbot_id_5_m
GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
Concept ACK on that.
Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
0[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o 1/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=] 2 5603 | 3/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=] 4 8254 | 5/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:9840:2: warning: trailing whitespace [-Wtrailing-whitespace=]Will need to resolve that somehow.
I’ll look into it.
GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
Concept ACK on that.
Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
0[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o 1/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=] 2 5603 | 3/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=] 4 8254 | 5/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:9840:2: warning: trailing whitespace [-Wtrailing-whitespace=]Will need to resolve that somehow.
I’ll look into it.
Addressed in #32648.
-Wtrailing-whitespace for now. That could be added when Qts tools are fixed.
GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable
-Wleading-whitespace. Unfortunately, CMake/Qt will generate code that contains trailing whitespace:0[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o 1/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=] 2 5603 | 3/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=] 4 8254 | 5/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:9840:2: warning: trailing whitespace [-Wtrailing-whitespace=]So
-Wtrailing-whitespacecould be enabled later, once Qts tooling is fixed.
CMake 3.27 and newer handle this. The CMP0151 policy must be set to NEW.
ACK 5e2620443d13b58efb38310263d67e2b823d6a1e.
The first two commits were reviewed using git diff --ignore-space-change. Commit f2385191520f35fdef07743e5a12feaa87085dbe additionally removes an empty line, which is OK.
So
-Wtrailing-whitespacecould be enabled later, once Qts tooling is fixed.CMake 3.27 and newer handle this. The
CMP0151policy must be set toNEW.
As it is reasonable to expect CMake ≥3.27 on systems with GCC 15, I believe -Wtrailing-whitespace can be reintroduced.
1028 ; addm [mem], reg
1029 ; Add reg to mem using reg-mem add and store
1030 %macro addm 2
1031- add %2, %1
1032- mov %1, %2
1033+ add %2, %1
968@@ -969,27 +969,27 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
969
970 /*
971 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
972-; Copyright (c) 2012, Intel Corporation
973-;
974-; All rights reserved.
975-;
976+; Copyright (c) 2012, Intel Corporation
reACK 854269a785fc51602cc5ae018c666fc1d2659776
git range-diff b271b34...854269a indicates it’s a simple rebase.
Concept ACK. The large commits here should be scripted-diffs though. I believe the following works, if expand/sponge are available:
0scripted-diff: univalue: change leading whitespace to spaces
1
2-BEGIN VERIFY SCRIPT-
3sed -i 's/^\t/ /' src/univalue/include/univalue_escapes.h
4-END VERIFY SCRIPT-
0scripted-diff: crypto: remove tabs and trailing whitespace in sha256_sse4.cpp
1
2-BEGIN VERIFY SCRIPT-
3expand -t4 src/crypto/sha256_sse4.cpp | sed 's/ *$//' | sponge src/crypto/sha256_sse4.cpp
4-END VERIFY SCRIPT-
Only diff with what you’ve got is:
0$ git diff HEAD pull/origin/32482
1diff --git a/src/crypto/sha256_sse4.cpp b/src/crypto/sha256_sse4.cpp
2index 2d37d124d1e..96f39e2df02 100644
3--- a/src/crypto/sha256_sse4.cpp
4+++ b/src/crypto/sha256_sse4.cpp
5@@ -1078,7 +1078,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
6 %define e r8d
7
8 %endif
9-%define TBL rbp
10+%define TBL rbp
11 %define a eax
12 %define b ebx
reACK 2e713c22c67b3551f04273428db266503178e7fa
0git fetch origin 2e713c22c67b3551f04273428db266503178e7fa && git range-diff 854269a...2e713c2
commit messages include scripted diffs and the suggested change by AJ:
0+%define TBL rbp
1145@@ -1146,7 +1146,7 @@ inline void Popen::execute_process() noexcept(false)
1146 NULL, // process security attributes
1147 NULL, // primary thread security attributes
1148 TRUE, // handles are inherited
1149- creation_flags, // creation flags
1150+ creation_flags, // creation flags
if expand/sponge are available:
We don’t have sponge, but added a commit to add moreutils, and pushed up the scripted diffs.
Tested 2e713c22c67b3551f04273428db266503178e7fa on Ubuntu 25.10 with GCC 15.2.0:
0$ cmake --build build
1<snip>
2[340/1092] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp.o
3src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=]
4 5603 |
5src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=]
6 8254 |
7src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:9840:2: warning: trailing whitespace [-Wtrailing-whitespace=]
8 9840 |
9src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:11671:2: warning: trailing whitespace [-Wtrailing-whitespace=]
1011671 |
11src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:11880:2: warning: trailing whitespace [-Wtrailing-whitespace=]
1211880 |
13src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:32981:2: warning: trailing whitespace [-Wtrailing-whitespace=]
1432981 |
15src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:33585:2: warning: trailing whitespace [-Wtrailing-whitespace=]
1633585 |
17src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:34942:2: warning: trailing whitespace [-Wtrailing-whitespace=]
1834942 |
19src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:35370:2: warning: trailing whitespace [-Wtrailing-whitespace=]
2035370 |
21[348/1092] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
22src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:82738:2: warning: trailing whitespace [-Wtrailing-whitespace=]
2382738 |
24src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:93715:2: warning: trailing whitespace [-Wtrailing-whitespace=]
2593715 |
26src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:97857:2: warning: trailing whitespace [-Wtrailing-whitespace=]
2797857 |
28src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:204993:2: warning: trailing whitespace [-Wtrailing-whitespace=]
29204993 |
30src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:212562:2: warning: trailing whitespace [-Wtrailing-whitespace=]
31212562 |
32src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:228433:2: warning: trailing whitespace [-Wtrailing-whitespace=]
33228433 |
34src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:228777:2: warning: trailing whitespace [-Wtrailing-whitespace=]
35228777 |
36src/qt/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp:228933:2: warning: trailing whitespace [-Wtrailing-whitespace=]
37228933 |
38[419/1092] Building CXX object src/CMakeFiles/minisketch.dir/minisketch/src/fields/clmul_4bytes.cpp.o
39In file included from ../src/minisketch/src/fields/clmul_4bytes.cpp:12:
40../src/minisketch/src/fields/clmul_common_impl.h:39:66: warning: trailing whitespace [-Wtrailing-whitespace=]
41 39 | __m128i red1 = _mm_clmulepi64_si128(high1, MOD128, 0x00);
42[1092/1092] Linking CXX executable bin/fuzz
I guess we are now just blocked on Qt / GUI tooling…
From #32648 (comment):
this should be fixed in Qts tooling.
Even if it’s been fixed upstream, I don’t expect that change to be backported to all Qt versions down to 6.2. Therefore, we still need to suppress warnings in generated files. For example, as follows:
0--- a/src/qt/CMakeLists.txt
1+++ b/src/qt/CMakeLists.txt
2@@ -50,11 +50,20 @@ endfunction()
3
4 set(CMAKE_AUTOMOC ON)
5 set(CMAKE_AUTOMOC_MOC_OPTIONS "-p${CMAKE_CURRENT_SOURCE_DIR}")
6-set(CMAKE_AUTORCC ON)
7 set(CMAKE_AUTOUIC ON)
8 set(CMAKE_AUTOUIC_SEARCH_PATHS forms)
9
10+set(CMAKE_AUTORCC OFF)
11 configure_file(bitcoin_locale.qrc bitcoin_locale.qrc USE_SOURCE_PERMISSIONS COPYONLY)
12+qt6_add_resources(BITCOIN_QRC bitcoin.qrc)
13+qt6_add_resources(BITCOIN_LOCALE_QRC ${CMAKE_CURRENT_BINARY_DIR}/bitcoin_locale.qrc)
14+set_property(
15+ SOURCE
16+ ${BITCOIN_QRC}
17+ ${BITCOIN_LOCALE_QRC}
18+ PROPERTY COMPILE_OPTIONS
19+ -Wno-trailing-whitespace
20+)
21
22 # The bitcoinqt sources have to include headers in
23 # order to parse them to collect translatable strings.
24@@ -119,8 +128,8 @@ add_library(bitcoinqt STATIC EXCLUDE_FROM_ALL
25 utilitydialog.h
26 $<$<PLATFORM_ID:Windows>:winshutdownmonitor.cpp>
27 $<$<PLATFORM_ID:Windows>:winshutdownmonitor.h>
28- bitcoin.qrc
29- ${CMAKE_CURRENT_BINARY_DIR}/bitcoin_locale.qrc
30+ ${BITCOIN_QRC}
31+ ${BITCOIN_LOCALE_QRC}
32 )
33 target_compile_definitions(bitcoinqt
34 PUBLIC
Even if it’s been fixed upstream,
It’s not clear to me if it has been fixed or not, can you link to the relevant change / issue? I can cherry-pick the change above in, if you create a commit, and add a comment inline, for when the change can be dropped.
Even if it’s been fixed upstream,
It’s not clear to me if it has been fixed or not, can you link to the relevant change / issue?
See https://bugreports.qt.io/browse/QTBUG-141858.
I can cherry-pick the change above in, if you create a commit, and add a comment inline, for when the change can be dropped.
Feel free to pick the top two commits from https://github.com/hebasto/bitcoin/commits/pr32482/1110/.
See https://bugreports.qt.io/browse/QTBUG-141858. Feel free to pick
Thanks. I’ve pulled those two in here.
Thanks. I’ve pulled those two in here.
Looking at the CI. Those commits are not going to work as implemented (essentially anywhere we are using Clang).
Thanks. I’ve pulled those two in here.
Looking at the CI. Those commits are not going to work as implemented (essentially anywhere we are using Clang).
Here is a branch with the fixed commit: https://github.com/hebasto/bitcoin/commits/pr32482/1110.1/.
Makes the sponge utility available for an upcoming scripted-diff.
-BEGIN VERIFY SCRIPT-
sed -i 's/^\t/ /' src/univalue/include/univalue_escapes.h
-END VERIFY SCRIPT-
Co-authored-by: Anthony Towns <aj@erisian.com.au>
-BEGIN VERIFY SCRIPT-
expand -t4 src/crypto/sha256_sse4.cpp | sed 's/ *$//' | sponge src/crypto/sha256_sse4.cpp
-END VERIFY SCRIPT-
Co-authored-by: Anthony Towns <aj@erisian.com.au>
See: https://github.com/arun11299/cpp-subprocess/pull/121.
This change provides finer control over the compilation of RCC-generated
files.
See: https://bugreports.qt.io/browse/QTBUG-141858.
This is available in GCC 15. See
https://gcc.gnu.org/gcc-15/changes.html.
462@@ -463,6 +463,9 @@ else()
463 try_append_cxx_flags("-Wself-assign" TARGET warn_interface SKIP_LINK)
464 try_append_cxx_flags("-Wbidi-chars=any" TARGET warn_interface SKIP_LINK)
465 try_append_cxx_flags("-Wundef" TARGET warn_interface SKIP_LINK)
466+ try_append_cxx_flags("-Wleading-whitespace=spaces" TARGET warn_interface SKIP_LINK)
467+ try_append_cxx_flags("-Wtrailing-whitespace=any" TARGET warn_interface SKIP_LINK)
468+
This is available in GCC 15. See
https://gcc.gnu.org/gcc-15/changes.html.
and I am not sure how many devs are using GCC-15.
I am :)
how many devs are using GCC-15
lately I’m also doing most of my benchmarks with 15
Not sure how to reproduce locally, but this seems to be failing:
0/__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
1 33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808327#step:6:405
Not sure how to reproduce locally, but this seems to be failing:
0/__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=] 1 33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808327#step:6:405
Does the following patch help:
0--- a/src/clientversion.cpp
1+++ b/src/clientversion.cpp
2@@ -30,7 +30,7 @@ const std::string UA_NAME("Satoshi");
3 // - "#define BUILD_GIT_COMMIT ...", if the top commit is not tagged
4 // - "// No build information available", if proper git information is not available
5
6-//! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives. $Format:%n#define GIT_COMMIT_ID "%H"$
7+//! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.$Format:%n#define GIT_COMMIT_ID "%H"$
8
9 #ifdef BUILD_GIT_TAG
10 #define BUILD_DESC BUILD_GIT_TAG
?
Not sure how to reproduce locally, but this seems to be failing:
Interesting. My Fedora box, with the same GCC (15.2.1), does not fail the same as your CI job: https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808331.
Does the following patch help:
Yes, I presume it does help. I was just confused why the checkout GH action was using a git archive. Though, the guix release archive should be affected as well.