Skip to content

build: add -Wleading-whitespace #32482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

build: add -Wleading-whitespace #32482

wants to merge 5 commits into from
+614 −611

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented May 13, 2025 •
edited
Loading

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 13, 2025 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32482.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK l0rinc
Stale ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member

hebasto commented May 13, 2025

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:

[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=]
 5603 | 
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=]
 8254 | 
/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.

@fanquake fanquake mentioned this pull request May 13, 2025
Merged
fanquake added a commit that referenced this pull request May 16, 2025
@fanquake
Merge #32485: Update minisketch subtree
c521192 
bf25a09 Squashed 'src/minisketch/' changes from d1e6bb8bbf..ea8f66b1ea (fanquake)

Pull request description:

  Includes:
  * bitcoin-core/minisketch#89
  * bitcoin-core/minisketch#94 (in #32482)

ACKs for top commit:
  maflcko:
    lgtm ACK 46b533d
  willcl-ark:
    ACK 46b533d

Tree-SHA512: 7655aa480381c711fd3397fc7031be4619d5bd67f9d68f7e69f5f903734e776b97f7a2384257b365fc4e1f504b9c97acfd4a355840d1064d4c2c2f259070f4ef
@fanquake fanquake force-pushed the trailing_whitespace branch from 6f7fb28 to d09af54 Compare May 16, 2025 09:14
@hebasto hebasto mentioned this pull request May 30, 2025
Closed
@hebasto
Copy link
Member

hebasto commented May 30, 2025 •
edited
Loading

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:

[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=]
 5603 | 
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=]
 8254 | 
/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.

@fanquake fanquake force-pushed the trailing_whitespace branch from d09af54 to 93087de Compare July 2, 2025 13:03
@fanquake fanquake changed the title build: add -W[leading|trailing]-whitespace build: add -Wleading-whitespace Jul 2, 2025
@fanquake fanquake marked this pull request as ready for review July 2, 2025 13:07
@fanquake
Copy link
Member Author

fanquake commented Jul 2, 2025

Dropped -Wtrailing-whitespace for now. That could be added when Qts tools are fixed.

@fanquake fanquake force-pushed the trailing_whitespace branch from 93087de to 5e26204 Compare August 3, 2025 11:01
@hebasto
Copy link
Member

hebasto commented Aug 8, 2025

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:

[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing whitespace [-Wtrailing-whitespace=]
 5603 | 
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:8254:2: warning: trailing whitespace [-Wtrailing-whitespace=]
 8254 | 
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:9840:2: warning: trailing whitespace [-Wtrailing-whitespace=]

So -Wtrailing-whitespace could be enabled later, once Qts tooling is fixed.

CMake 3.27 and newer handle this. The CMP0151 policy must be set to NEW.

hebasto
hebasto approved these changes Aug 8, 2025
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5e26204.

The first two commits were reviewed using git diff --ignore-space-change. Commit f238519 additionally removes an empty line, which is OK.

So -Wtrailing-whitespace could be enabled later, once Qts tooling is fixed.

CMake 3.27 and newer handle this. The CMP0151 policy must be set to NEW.

As it is reasonable to expect CMake ≥3.27 on systems with GCC 15, I believe -Wtrailing-whitespace can be reintroduced.

@fanquake fanquake force-pushed the trailing_whitespace branch from 5e26204 to a2984c8 Compare August 13, 2025 09:41
@fanquake fanquake force-pushed the trailing_whitespace branch from a2984c8 to bed4247 Compare August 21, 2025 11:29
@fanquake fanquake force-pushed the trailing_whitespace branch from bed4247 to 8c60dc3 Compare August 29, 2025 09:21
l0rinc
l0rinc approved these changes Aug 29, 2025
View reviewed changes
Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core review ACK 8c60dc3

@DrahtBot DrahtBot requested a review from hebasto August 29, 2025 19:57
@fanquake fanquake mentioned this pull request Sep 1, 2025
Merged
@l0rinc
Copy link
Contributor

l0rinc commented Sep 1, 2025

ACK b271b34

fanquake added a commit to bitcoin-core/minisketch that referenced this pull request Sep 2, 2025
@fanquake
Merge #98: misc: drop trailing whitespace
d1bd01e 
fe2c88a misc: drop trailing whitespace (fanquake)

Pull request description:

  This came up downstream: bitcoin/bitcoin#32482 (comment).

ACKs for top commit:
  l0rinc:
    ACK fe2c88a

Tree-SHA512: db6e141c19b20e92e85d3dc21bd90aa6715260a739b87b0ee21a3865b30c52c92a1c09217a3b3090a3ee10499b5dca312a58fe5dfcec32044c447ce55a57c607
@bitcoin bitcoin deleted a comment from DrahtBot Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hebasto hebasto Awaiting requested review from hebasto

1 more reviewer

@l0rinc l0rinc l0rinc approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fanquake @DrahtBot @hebasto @l0rinc @maflcko