Page MenuHomePhabricator
Differential D4736

Merge #13534: Don't assert(foo()) where foo() has side effects
ClosedPublic
Actions

Authored by nakihito on Dec 16 2019, 21:06.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING46cdf26c9fd6: Merge #13534: Don't assert(foo()) where foo() has side effects
rABC46cdf26c9fd6: Merge #13534: Don't assert(foo()) where foo() has side effects
Summary

6ad0328f1c Don't assert(foo()) where foo has side effects (practicalswift)

Pull request description:

Don't `assert(foo())` where `foo` has side effects.

From `assert(3)`:

> If the macro `NDEBUG` is defined at the moment `<assert.h>` was last included, the macro `assert()` generates no code, and hence does nothing at all.

Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that.

Tree-SHA512: 28cff0c6d1c2fb612ca58c9c94142ed01c5cfd0a2fecb8e59cdb6c270374b215d952ed3491d921d84dc1b439fa49da4f0e75e080f6adcbc6b0e08be14e54c170

Partial backport of Core PR13534
https://github.com/bitcoin/bitcoin/pull/13534/

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

nakihito created this revision.Dec 16 2019, 21:06
Owners added a reviewer: Restricted Owners Package.Dec 16 2019, 21:06
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 16 2019, 21:06
nakihito planned changes to this revision.Dec 16 2019, 21:06
teamcity added a comment.Dec 16 2019, 21:06

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Dec 16 2019, 21:21

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8576: Diff 14882.Dec 16 2019, 21:22
nakihito requested review of this revision.Dec 17 2019, 00:37

Review note:
https://github.com/bitcoin/bitcoin/pull/13534/files#diff-d546634098d8003b6397c8643b74e47dL427 and https://github.com/bitcoin/bitcoin/pull/13534/files#diff-bac18fdd925f2a450e0ddcfab802d96eL44 were excluded because we do not have that code in our code base. The former was removed in D3136 and the latter was not backported.

Fabien accepted this revision.Dec 17 2019, 10:32
Fabien added inline comments.
src/bench/checkblock.cpp
58 ↗(On Diff #14882)

nit: rename ret => checked to avoid merge conflicts

This revision is now accepted and ready to land.Dec 17 2019, 10:32
nakihito updated this revision to Diff 14918.Dec 17 2019, 18:04

ret -> checked.

teamcity added a comment.Dec 17 2019, 18:04

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Dec 17 2019, 18:15

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8601: Diff 14918.Dec 17 2019, 18:15
Closed by commit rABC46cdf26c9fd6: Merge #13534: Don't assert(foo()) where foo() has side effects (authored by MarcoFalke <falke.marco@gmail.com>, committed by nakihito). · Explain WhyDec 17 2019, 18:21
This revision was automatically updated to reflect the committed changes.
nakihito added a commit: rABC46cdf26c9fd6: Merge #13534: Don't assert(foo()) where foo() has side effects.
markblundeberg mentioned this in D4912: Reintroduce IsSolvable.Jan 12 2020, 12:48
Mark Lundeberg <markblundeberg@users.noreply.github.com> mentioned this in rABC9dda12ee9286: Reintroduce IsSolvable.Jan 13 2020, 00:59
nakihito added a commit: rSTAGING46cdf26c9fd6: Merge #13534: Don't assert(foo()) where foo() has side effects.Apr 7 2020, 22:06
Mark Lundeberg <markblundeberg@users.noreply.github.com> mentioned this in rSTAGING9dda12ee9286: Reintroduce IsSolvable.Apr 7 2020, 22:23

Revision Contents
Changeset List

PathSize
src/
bench/
10 lines
6 lines
test/
2 lines

Diff 14920

View Options

src/bench/checkblock.cpp

Loading...
View Options

src/httprpc.cpp

Loading...
View Options

src/test/txvalidation_tests.cpp

Loading...