Page MenuHomePhabricator

Don't assert(...) with side effects
ClosedPublic

Authored by Fabien on Jun 10 2019, 13:09.

Details

Summary

This diffs fixes the assertions introduced in D3232 and adds a linter to
avoid reproducing the issue.

The fix is a backport of core PR14088
https://github.com/bitcoin/bitcoin/pull/14088/files
The linter is based on the second commit, but integrated into arcanist.

Test Plan
./src/test/test_bitcoin -t scheduler_tests

Revert the changes in scheduler_tests and run:

arc lint -- src/test/scheduler_tests.cpp

The linter should catch the errors

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR14088
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6252
Build 10551: Bitcoin ABC Buildbot (legacy)
Build 10550: arc lint + arc unit

Event Timeline

jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
arcanist/linter/AssertWithSideEffectsLinter.php
63 ↗(On Diff #9289)

Nit: It may improve this message to refer to what the original comment did in test/lint/lint-assertions.sh:

# PRE31-C (SEI CERT C Coding Standard):
# "Assertions should not contain assignments, increment, or decrement operators."
This revision is now accepted and ready to land.Jun 10 2019, 16:43

Rebase and update error message.