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
feature-backport-95801902b
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7398
Build 12839: Bitcoin ABC Buildbot (legacy)
Build 12838: 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

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

51

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 ↗(On Diff #11371)

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 ↗(On Diff #11371)

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

src/compat/assumptions.h
32 ↗(On Diff #11371)

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 ↗(On Diff #11371)

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