Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 13, 2018

  • 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 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

@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

@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

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants