Page MenuHomePhabricator

Return void instead of bool for functions that cannot fail
ClosedPublic

Authored by fpelliccioni on Mon, Sep 30, 15:08.

Details

Summary

Return void instead of bool for functions that cannot fail:

  • CBlockTreeDB::ReadReindexing(...)
  • CChainState::ResetBlockFailureFlags(...)
  • CTxMemPool::addUnchecked(...)
  • CWallet::CommitTransaction(...)
  • CWallet::LoadDestData(...)
  • CWallet::LoadKeyMetadata(...)
  • CWallet::LoadScriptMetadata(...)
  • CWallet::LoadToWallet(...)
  • CWallet::SetHDChain(...)
  • CWallet::SetHDSeed(...)
  • PendingWalletTx::commit(...)
  • RemoveLocal(...)
  • SetMinVersion(...)
  • StartHTTPServer(...)
  • StartRPC(...)
  • TorControlConnection::Disconnect(...)

Backport of Bitcoin Core PR13774
https://github.com/bitcoin/bitcoin/pull/13774

Test Plan
make check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Mon, Sep 30, 15:08
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Sep 30, 15:08
jasonbcox requested changes to this revision.Mon, Sep 30, 16:33
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/validation.cpp
3239 ↗(On Diff #13274)

This function can return false (and see build failure).

3264 ↗(On Diff #13274)

ditto

This revision now requires changes to proceed.Mon, Sep 30, 16:33
fpelliccioni updated this revision to Diff 13320.Wed, Oct 2, 14:15

fixes building errors.

I see that you use make check-all in many tests. If you are using cmake, you should use ninja as it is faster. But right now, cmake is only a secondary option, so make sure the autotool build works.

deadalnix added inline comments.Wed, Oct 2, 21:00
src/validation.cpp
3239 ↗(On Diff #13274)

The function you have the comment on, yes. Hopefully, this isn't the function that is being changed.

deadalnix accepted this revision.Wed, Oct 2, 21:08

The test plan is meant to actually be ran.

jasonbcox accepted this revision.Thu, Oct 3, 00:24
This revision is now accepted and ready to land.Thu, Oct 3, 00:24

I see that you use make check-all in many tests. If you are using cmake, you should use ninja as it is faster. But right now, cmake is only a secondary option, so make sure the autotool build works.

I usually use both ways to compile.
Unfortunately on my computer, ninja is not much faster than make.

The test plan is meant to actually be ran.

Yes, I always run the test plan.
I really don't know what happened to me here.
Sorry.