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

Qt: Replace remaining 0 with nullptr #15114

Merged
merged 2 commits into from Jan 14, 2019

Conversation

Projects
None yet
8 participants
@Empact
Copy link
Member

commented Jan 6, 2019

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

These changes are extracted from #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.

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

@@ -338,7 +338,7 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& er
BerkeleyEnvironment* env = GetWalletEnv(file_path, walletFile);
fs::path walletDir = env->Directory();

LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(nullptr, nullptr, nullptr));
@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Concept ACK.

@@ -300,8 +300,8 @@ QVariant AddressTableModel::headerData(int section, Qt::Orientation orientation,

Qt::ItemFlags AddressTableModel::flags(const QModelIndex &index) const
{
if(!index.isValid())
return 0;
if (!index.isValid()) return Qt::NoItemFlags;

This comment has been minimized.

@@ -45,7 +45,7 @@ class ShutdownWindow : public QWidget
Q_OBJECT

public:
explicit ShutdownWindow(QWidget *parent=0, Qt::WindowFlags f=0);
explicit ShutdownWindow(QWidget *parent=nullptr, Qt::WindowFlags f=Qt::Widget);

This comment has been minimized.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Rationale? Changing code style stuff for no reason isn't useful...

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Concept ACK

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

@luke-jr The change @Empact is suggesting is not a code style change.

Reasons to use nullptr include:

  • nullptr has a well-specified (very restrictive) type, and thus works in more scenarios where type deduction might do the wrong thing on NULL or 0.
  • Improves readability.
  • Minimises surprises: nullptr cannot be confused with an int.

Enabling -Wzero-as-null-pointer-constant will help developers follow our developer guide which is a good thing. This in turn means that human reviewers will not have to point out the benefits of nullptr ever again :-)

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

utACK 60a2a0b24275757dee767deafadd86d24889bdf8

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

utACK 60a2a0b24275757dee767deafadd86d24889bdf8.
nit: could clang-format-diff.py be applied?

@promag

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Concept ACK, agree it improves code.

I don't think this should be merged without something to prevent more zero-as-null-pointer.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

@promag That is taken care of in #15112, right? :-)

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

I don't think this should be merged without something to prevent more zero-as-null-pointer.

I agree, as discussed many times, we don't want PR after PR fixing 'straggler' occurrences. It's not that important that it warrants some kind of crusade.

But if #15112 enables failing the build on this I'm okay with that and they should probably be merged together.

@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:

  • #15157 (rpc: Bumpfee units change, satoshis to BTC by benthecarman)
  • #15153 (gui: Add Open Wallet menu by promag)
  • #15101 (gui: Add WalletController by promag)
  • #15040 (qt: Add workaround for QProgressDialog bug on macOS by hebasto)
  • #13880 (gui: Try shutdown also on failure by MarcoFalke)
  • #12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)

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.

practicalswift and others added some commits Jul 30, 2018

Replace remaining 0 with nullptr in Qt code
Also used type-appropriate enum values such as Qt::NoItemFlags in
some cases.

All cases identified via -Wzero-as-null-pointer-constant

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

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

Rebased for #13216

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

utACK 3a0e76f

1 similar comment
@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

utACK 3a0e76f

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

@laanwj laanwj merged commit 3a0e76f into bitcoin:master Jan 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.