Page MenuHomePhabricator

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

Authored by fpelliccioni on Wed, Sep 11, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Wed, Sep 11, 18:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Sep 11, 18:13
Fabien requested changes to this revision.Wed, Sep 11, 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.Wed, Sep 11, 19:33
fpelliccioni updated this revision to Diff 11371.Tue, Sep 17, 19:41

Fixed following Fabien suggestions.

Fabien requested changes to this revision.Wed, Sep 18, 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.Wed, Sep 18, 08:46
fpelliccioni added inline comments.Wed, Sep 18, 18:05
src/compat/assumptions.h
32 ↗(On Diff #11371)

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

fpelliccioni added inline comments.Wed, Sep 18, 18:08
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?

fpelliccioni updated this revision to Diff 13031.Fri, Sep 20, 13:16

Removes obvious type checks.

Fabien added inline comments.Fri, Sep 20, 13:37
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.

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