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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Jun 10 2019, 13:09
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 10 2019, 13:09
jasonbcox accepted this revision.Jun 10 2019, 16:43
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
Fabien updated this revision to Diff 9294.Jun 10 2019, 20:18

Rebase and update error message.

This revision was automatically updated to reflect the committed changes.