Page MenuHomePhabricator

Add compile time verification of assumptions we're currently making implicitly/tacitly
ClosedPublic

Authored by fpelliccioni on Sep 11 2019, 18:13.

Details

Summary

Add compile time verification of assumptions we're currently making implicitly/tacitly.

Backport of Bitcoin Core PR15391
https://github.com/bitcoin/bitcoin/pull/15391

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D4043
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7497
Build 13037: Bitcoin ABC Buildbot (legacy)
Build 13036: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Sep 11 2019, 19:33
Fabien added inline comments.
src/compat/assumptions.h
18 ↗(On Diff #11213)

This is duplicated, you should remove it from validation.cpp

51 ↗(On Diff #11213)

IMO a list of things that doesn't exist shouldn't exist... but keep it to avoid merge conflicts.

This revision now requires changes to proceed.Sep 11 2019, 19:33

Fixed following Fabien suggestions.

Fabien requested changes to this revision.Sep 18 2019, 08:46
Fabien added inline comments.
src/compat/assumptions.h
32

This one is useless.
It makes somewhat sense for core as they used unsigned char, but because we are enforcing uint8_t this is not a concern for us, there is no such assumption here.

This revision now requires changes to proceed.Sep 18 2019, 08:46
src/compat/assumptions.h
32

Ah! because the Linter replaced unsigned char by uint8_t, right?

src/compat/assumptions.h
32

Do you prefer to maintain the assertion as a code comment? Or do you prefer to remove the line?

Removes obvious type checks.

src/compat/assumptions.h
32

Yes, because we are enforcing 8 bits through the use of uint8_t there is no assumption made, and this check can be removed.

This revision is now accepted and ready to land.Sep 20 2019, 14:28