Page MenuHomePhabricator

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

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

Details

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

Owners added a reviewer: Restricted Owners Package.Dec 16 2019, 21:06

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