Skip to content

serialize: Document integer width assumptions we are making when calculating compact sizes#14475

Closed
practicalswift wants to merge 1 commit intobitcoin:masterfrom
practicalswift:integer-width-assumptions
Closed

serialize: Document integer width assumptions we are making when calculating compact sizes#14475
practicalswift wants to merge 1 commit intobitcoin:masterfrom
practicalswift:integer-width-assumptions

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 13, 2018 •
edited
Loading

  • Document integer width assumptions we are making when calculating compact sizes
  • Document floating-point width assumptions we are making in the serialization code

Rationale:

  • Explicit is better than implicit
  • Compile-time failure is better than run time failure :-)

@practicalswift practicalswift force-pushed the integer-width-assumptions branch from 745b01f to 82cea43 Compare October 13, 2018 14:51
@practicalswift practicalswift force-pushed the integer-width-assumptions branch from 82cea43 to 4486f2a Compare October 13, 2018 15:03
@practicalswift practicalswift changed the title serialize: Document integer width assumptions we are making when calculating compact sizes serialize: Document width assumptions we are making in the serialization code Oct 13, 2018
@hebasto
Copy link
Member

hebasto commented Oct 13, 2018

utACK 25ae1a1

@fanquake fanquake mentioned this pull request Oct 14, 2018
Closed
@practicalswift practicalswift force-pushed the integer-width-assumptions branch from 25ae1a1 to 4486f2a Compare October 14, 2018 11:34
@practicalswift practicalswift changed the title serialize: Document width assumptions we are making in the serialization code serialize: Document integer width assumptions we are making when calculating compact sizes Oct 14, 2018
@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 14, 2018

@hebasto Updated. Please re-review :-)

@hebasto
Copy link
Member

hebasto commented Oct 14, 2018

Let's assume a platform with sizeof(short) > 2 or sizeof(int) > 4.
Without static_assert statement the logic will remain correct if
nSize <= std::numeric_limits<unsigned short>::max() replace with nSize <= 0xffff
and
nSize <= std::numeric_limits<unsigned int>::max() replace with nSize <= 0xffffffff

@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 14, 2018 •
edited
Loading

@hebasto As I read the code more adjustments would be needed to the serialisation code to make it work properly under such a platform. I don't think changing GetSizeOfCompactSize and WriteCompactSize would be enough?

FWIW I think the platform requirements are reasonable. My goal with this PR is just making the platform requirements explicit and immediately obvious to compilers, humans and static analysers :-)

I think a compile time check is the most elegant and non-intrusive way to achieve that goal.

@hebasto
Copy link
Member

hebasto commented Oct 15, 2018

FWIW I think the platform requirements are reasonable. My goal with this PR is just making the platform requirements explicit and immediately obvious to compilers, humans and static analysers :-)

I think a compile time check is the most elegant and non-intrusive way to achieve that goal.

Agree. ACK 4486f2a

@practicalswift practicalswift deleted the integer-width-assumptions branch April 10, 2021 19:36
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@practicalswift @hebasto @fanquake