Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Don't assert(...) with side effects #14088

Merged
merged 2 commits into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@practicalswift
Copy link
Member

commented Aug 28, 2018

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

From the developer notes:

Assertions should not have side-effects

Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

These assertions were introduced quite recently (in #14069 which was merged two days ago) and since this is a recurring thing (see #13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

@practicalswift practicalswift changed the title tests: Don't `assert(...)` with side effects tests: Don't assert(...) with side effects Aug 28, 2018

@fanquake fanquake added the Tests label Aug 28, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

utACK 1916b4e

@skeees

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Thanks :) utACK 1916b4e
I don't know how I feel about the linter though - it is too narrowly scoped to ++|-- and won't catch anything inside of a BOOST_* style assert either.

If you're worried about somebody trying to run a node with NDEBUG - maybe add a more explanatory warning message https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L51 about why this could dangerous - instead of the very vague one that's there now?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@skeees Yes, the regression test will only catch this specific type of side effect. The regression test is not meant to be exhaustive (regression tests seldom are! :-)).

FWIW, this is the third time this specific type of side effect is fixed during the past few months so having a check for this is better than no checking :-)

@practicalswift practicalswift force-pushed the practicalswift:assertions-with-side-effects branch to ca1a093 Aug 28, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@skeees Updated version based on your feedback with improved comments. Now catching also accidental assignment operations in assertions :-)

From PRE31-C (SEI CERT C Coding Standard):

Assertions should not contain assignments, increment, or decrement operators.

Now catching all three.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@skeees Regarding BOOST_ASSERT(...) – we're not using it are we? :-)

@skeees

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

Cool - utACK - I think this change makes things strictly better. Regarding BOOST_CHECK_* stuff - used in unit tests but not production code I believe

I do think its worth changing the #error warning message (or adding a comment nearby) to something that explains why bitcoin cannot be compiled without assertions. Right now it just states so without any reason. Perhaps:

In addition to providing important sanity checks on the internal state of the system, is possible that assert statements throughout bitcoin may contain side-effecting code that is consensus critical, therefore bitcoin may not be compiled without assertions

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@skeees BOOST_CHECK_* is ever used outside of the tests:

$ git grep BOOST_CHECK_ ":(exclude)*/test/*"
$

And I don't see any scenario where BOOST_CHECK_* could be removed at run-time like assert(…) under NDEBUG. Do you? :-)

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

utACK ca1a093

And I don't see any scenario where BOOST_CHECK_* could be removed at run-time like assert(…) under NDEBUG. Do you? :-)

No—the problem does not exist for BOOST_CHECK_*, leave those alone please.

@laanwj laanwj merged commit ca1a093 into bitcoin:master Aug 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 31, 2018

Merge #14088: tests: Don't assert(...) with side effects
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

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

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in #14069 which was merged two days ago) and since this is a recurring thing (see #13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.