Skip to content
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.

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

build: Optionally enable -Wzero-as-null-pointer-constant #15112

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Empact
Copy link
Member

commented Jan 5, 2019

Note: This is based on #14920

This enables -Wzero-as-null-pointer-constant, while avoiding applying it to dependencies.

Used Qt::NoItemFlags, Qt::Widget instead where applicable.

See #10483, #10645, #13802 for more on nullptr.

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 5, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Concept ACK and in accordance with our developer notes.

Thanks for doing this!

Using zero as null pointer constant is sloppy at best and dangerous at worst :-)

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch 2 times, most recently Jan 6, 2019

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Concept ACK.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Could submit the src/qt changes as separate pull request, since those are the bulk of the changes?

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

@MarcoFalke Perhaps 10f81e916d5fbb2751d03d06901f68d9b507807c (including the two non QT fixes) can be posted as a separate PR since they are all trivial to review?

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@Empact What about -Werror=zero-as-null-pointer-constant as discussed in #15114 (comment)?

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 7, 2019

@Empact Empact changed the title build: Enable -Wzero-as-null-pointer-constant build: Enable -Werror=zero-as-null-pointer-constant Jan 7, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Enabled now. 👍

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

utACK 873145a56315ba032aa100646d682ec31d39e8d9

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 9, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

As per conversation with @theuni, this now is only active if the user configures with --enable-isystem, so the build possibilities are:
default: disabled
--enable-isystem: -Wzero-as-null-pointer-constant
--enable-isystem --enable-werror: -Werror=zero-as-null-pointer-constant

The idea being to add a travis run that exercises the latter combination to deny the introduction of violations.
#14920 (comment)

@Empact Empact changed the title build: Enable -Werror=zero-as-null-pointer-constant build: Optionally enable -Wzero-as-null-pointer-constant Jan 9, 2019

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 9, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16224 (gui: Bilingual GUI error messages by hebasto)
  • #15993 (net: Drop support of the insecure miniUPnPc versions by hebasto)
  • #15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)
  • #15421 (torcontrol: Launch a private Tor instance when not already running by luke-jr)
  • #13728 (lint: Run the CI lint stage on mac by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 13, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 13, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

Rebased for #13216

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 14, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Rebased for #15154

@DrahtBot DrahtBot removed the Needs rebase label Jan 14, 2019

benthecarman pushed a commit to benthecarman/bitcoin that referenced this pull request Jan 14, 2019

Merge bitcoin#15114: Qt: Replace remaining 0 with nullptr
3a0e76f Replace remaining 0 with nullptr in Qt code (Ben Woosley)
9096276 Don't use zero as null pointer constant (-Wzero-as-null-pointer-constant) (practicalswift)

Pull request description:

  This corrects all violations of `-Wzero-as-null-pointer-constant` identified in the Qt codebase.

  These changes are extracted from bitcoin#15112 as suggested by @MarcoFalke to ease review. This is in service of enabling `-Wzero-as-null-pointer-constant`, which should eliminate this as a concern going forward.

  Note there are 2 non-Qt changes: `src/test/allocator_tests.cpp` and `src/wallet/db.cpp`.

Tree-SHA512: 206bd668802147ba42bc413c2d7d259cb59aca9ec1da74a6bf2ca3932e60ae492faacbc61bcee0fd6b4b49a4d59d075b7e5404f0526b36c47718f9b0587e7768

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 14, 2019

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch Jan 20, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

Rebased for #15175

@fanquake fanquake removed the Needs rebase label Jan 20, 2019

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch 2 times, most recently Jan 20, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

utACK b4996079cda89711c2b8b80adbbb06a80f7b9c86

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch 2 times, most recently to 794d0f0 Jan 23, 2019

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch from 794d0f0 to e49d43f Feb 9, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

Rebased due to conflicts in #15377

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch from e49d43f to c2b209e Mar 5, 2019

@Empact

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

Rebased for #15288

@DrahtBot DrahtBot removed the Needs rebase label Mar 5, 2019

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch 2 times, most recently from ec3794f to 586a564 Mar 7, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

FWIW: I've verified that a disassembly of the bitcoind binary built with this patch applied is identical to a disassembly of the bitcoind binary built against master (as expected).

Empact added some commits Nov 16, 2018

build: Optionally include dependency headers with -isystem
When configured with --enable-isystem.

Was necessary to split QT_INCLUDES into QT_INCLUDES and
QT_MOC_INCLUDES because moc does not understand -isystem, e.g.:

    Unknown options: isystem/usr/local/Cellar/qt/5.10.0_1/include/QtNetwork[...]

This does not convert all uses, but focuses on libraries which have triggered
warnings/errors when applying initial additional build checks: QT, Univalue, and Berkeley DB.
LevelDb requires additional measures as its code is compiled with the project warnings
via AM_CXXFLAGS.

Note -isystem should not be applied to /usr/include, see BITCOIN_SYSTEM_INCLUDE
for a helper to convert -I to -isystem with /usr/include excepted.
build: Enable -Wdocumentation if isystem is enabled
Or -Werror=documentation if isystem & werror are enabled.
@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I get tons of these warnings (almost on every file):

/home/user/src/bitcoin/src/tinyformat.h:119:32: warning: unknown warning group '-Wzero-as-null-pointer-constant', ignored [-Wunknown-pragmas]
#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
                               ^

This is with clang version 3.8.1-24 (tags/RELEASE_381/final) (which is old as the hills but we apparently still support).

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch 2 times, most recently from 25f2d66 to f63f86f Jun 6, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Failing in travis with

In file included from ./index/base.h:8:0,
                 from index/base.cpp:6:
./dbwrapper.h:16:10: fatal error: leveldb/db.h: No such file or directory
 #include <leveldb/db.h>

Please update OP as it seems outdated.

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

Yeah @promag this is wip: rebasing and addressing @laanwj’s comment

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch 2 times, most recently from a8479ad to cad0c4d Jun 10, 2019

build: Enable -Wzero-as-null-pointer-constant under isystem
And -Werror=zero-as-null-pointer-constant if isystem & werror are enabled.

While disabling it in the following dependencies:
* LevelDb via build flags
* Tinyformat via pragmas
* Protobuf generated files via pragmas inserted via src/Makefile.am

@Empact Empact force-pushed the Empact:zero-as-null-pointer-constant branch from cad0c4d to 0dc0eec Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.