Paths

Table of Contentst

Differential D4043

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

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

Tags

None

Subscribers

None

Details

Reviewers

Fabien
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING9f466fe7a8ec: Add compile time verification of assumptions we're currently making…
rABC9f466fe7a8ec: Add compile time verification of assumptions we're currently making…

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

fpelliccioni created this revision.Sep 11 2019, 18:13

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 11 2019, 18:13

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7398: Diff 11213.Sep 11 2019, 18:28

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7497: Diff 11371.Sep 17 2019, 19:55

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

fpelliccioni added inline comments.Sep 18 2019, 18:05

src/compat/assumptions.h
32	Ah! because the Linter replaced `unsigned char` by `uint8_t`, right?

fpelliccioni added inline comments.Sep 18 2019, 18:08

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7522: Diff 13031.Sep 20 2019, 13:36

Fabien added inline comments.Sep 20 2019, 13:37

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.

Fabien accepted this revision.Sep 20 2019, 14:28

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

Closed by commit rABC9f466fe7a8ec: Add compile time verification of assumptions we're currently making… (authored by practicalswift <practicalswift@users.noreply.github.com>, committed by fpelliccioni). · Explain WhySep 20 2019, 18:17

This revision was automatically updated to reflect the committed changes.

fpelliccioni added a commit: rABC9f466fe7a8ec: Add compile time verification of assumptions we're currently making….

fpelliccioni added a commit: rSTAGING9f466fe7a8ec: Add compile time verification of assumptions we're currently making….Sep 27 2019, 22:46

Revision Contents
Changeset List

Path

Size

src/

1 line

compat/

53 lines

1 line

4 lines

Diff 11371

src/Makefile.am

Loading...

src/compat/assumptions.h

Loading...

src/util.h

Loading...

src/validation.cpp

Loading...