Validate assert() calls to be removable. #232

Closed
zander opened this Issue Feb 20, 2017 · 2 comments

Comments

Projects
None yet
1 participant
@zander

zander commented Feb 20, 2017

In almost all software the usage of 'assert()' statements are purely for the programmer, a way to make sure assumptions that the programmer makes while coding are correct during operations.

It is a very good rule to make sure that such statements can be removed from code without affecting the actual execution of the code.
What this means is that you have to write;

    bool ok = crucialOperation();
    assert(ok);

instead of

    assert(crucialOperation());

because if you remove the assert, the code will stop working in the second case because the call to crucial operation is also removed.

In Bitcoin, unlike most software, the asserts are NOT removed in production code and when I recently found the above example[1] in code, I got worried that there may be more calls like this in the code.
In line with the philosophy that code should have the least surprises, we should check all asserts to be in line with industry professionals expect.

  1. Example fix;
    dfe6d3a
@zander

This comment has been minimized.

Show comment
Hide comment
@zander

zander Jun 21, 2017

I think the only place these are still left are in the unit tests.
Unit tests should likely never use assert, instead they should use BOOST_CHECK()

I personally think its perfectly Ok for a unit test to first show a BOOST_CHECK fail and then follow with a crash if the code references a null pointer or similar. To the test-runner this is practically the same as what an assert does anyway.
In either case, it is a call to action :)

zander commented Jun 21, 2017

I think the only place these are still left are in the unit tests.
Unit tests should likely never use assert, instead they should use BOOST_CHECK()

I personally think its perfectly Ok for a unit test to first show a BOOST_CHECK fail and then follow with a crash if the code references a null pointer or similar. To the test-runner this is practically the same as what an assert does anyway.
In either case, it is a call to action :)

@zander

This comment has been minimized.

Show comment
Hide comment
@zander

zander Jul 31, 2017

We got them all, and I today pushed the change to return sanity to assert. See bd29c60

zander commented Jul 31, 2017

We got them all, and I today pushed the change to return sanity to assert. See bd29c60

@zander zander closed this Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment