Page MenuHomePhabricator
Differential D3277

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

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

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING3a0a0966a1d1: Don't assert(...) with side effects
rABC3a0a0966a1d1: Don't assert(...) with side effects
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 6248
Build 10543: Bitcoin ABC Buildbot (legacy)
Build 10542: arc lint + arc unit

Event Timeline

Fabien created this revision.Jun 10 2019, 13:09
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 10 2019, 13:09
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Jun 10 2019, 13:09

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jun 10 2019, 13:15

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6248: Diff 9289.Jun 10 2019, 13:15
jasonbcox accepted this revision.Jun 10 2019, 16:43
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
arcanist/linter/AssertWithSideEffectsLinter.php
63

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.

teamcity added a comment.Jun 10 2019, 20:18

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jun 10 2019, 20:28

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6252: Diff 9294.Jun 10 2019, 20:28
Closed by commit rABC3a0a0966a1d1: Don't assert(...) with side effects (authored by practicalswift <practicalswift@users.noreply.github.com>, committed by Fabien). · Explain WhyJun 10 2019, 21:19
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC3a0a0966a1d1: Don't assert(...) with side effects.
Fabien added a commit: rSTAGING3a0a0966a1d1: Don't assert(...) with side effects.Sep 19 2019, 00:10

Revision Contents
Changeset List

Diff 9289

View Options

.arclint

Loading...
View Options

arcanist/.phutil_module_cache

Loading...
View Options

arcanist/__phutil_library_map__.php

Loading...
View Options

arcanist/linter/AssertWithSideEffectsLinter.php

Loading...
View Options

src/test/scheduler_tests.cpp

Loading...