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

Add converter between UInt/UInt64/Int/Int64/Float/Double and Bytes. #1341

Merged
merged 5 commits into from
Jan 3, 2025

Conversation

SyoujyoujiNaiki
Copy link
Contributor

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented Dec 19, 2024

Pull Request Test Coverage Report for Build 4519

Details

  • 0 of 24 (0.0%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 83.222%

Changes Missing Coverage Covered Lines Changed/Added Lines %
double/double.mbt 0 2 0.0%
float/float.mbt 0 2 0.0%
int/int.mbt 0 2 0.0%
int64/int64.mbt 0 2 0.0%
uint/uint.mbt 0 2 0.0%
uint64/uint64.mbt 0 2 0.0%
builtin/bytesview.mbt 0 12 0.0%
Totals Coverage Status
Change from base Build 4517: -0.4%
Covered Lines: 4717
Relevant Lines: 5668

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator

Maybe it would be better to have conversions with BytesView (after we add BytesView of course)

graph TD;

Data --> Bytes;
Bytes --> BytesView;
BytesView --> Data;
Loading

@SyoujyoujiNaiki
Copy link
Contributor Author

Maybe it would be better to have conversions with BytesView (after we add BytesView of course)

graph TD;

Data --> Bytes;
Bytes --> BytesView;
BytesView --> Data;
Loading

OK. Should I close this PR for now?

@peter-jerry-ye
Copy link
Collaborator

It will come soon, so we can leave this PR as it is now. Or we can merge the data -> bytes part first.

@peter-jerry-ye
Copy link
Collaborator

Can you please rebase to #1394 ?

Copy link

peter-jerry-ye-code-review bot commented Jan 3, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations from the provided git diff output:

  1. Inconsistent Documentation Style:

    • The documentation comments for the newly added functions (e.g., to_be_bytes, to_le_bytes) use ///| as the comment delimiter. However, in some files (e.g., float.mbt), the documentation comments for existing functions use ///. This inconsistency in comment style should be standardized across the codebase for better readability and maintainability.
  2. Potential Overflow Risk:

    • In the to_be_bytes and to_le_bytes functions for UInt and UInt64, there is no explicit handling of overflow when shifting bits (e.g., self >> 56). While this might not be an issue in practice due to the types being unsigned, it would be safer to add assertions or checks to ensure the input values are within the expected range before performing bitwise operations.
  3. Missing Error Handling:

    • The to_uint_be, to_uint_le, to_uint64_be, and to_uint64_le functions in bytesview.mbt assume that the BytesView is exactly 4 or 8 bytes long. If the input BytesView is shorter or longer than expected, these functions will likely fail or produce incorrect results. It would be better to add validation to ensure the input length matches the expected size before performing the conversion.

These issues should be addressed to improve code quality and robustness.

Comment on lines 208 to 215
pub fn to_be_bytes(self : Double) -> BytesView {
self.reinterpret_as_uint64().to_be_bytes_view()
}

///|
pub fn to_le_bytes(self : Double) -> BytesView {
self.reinterpret_as_uint64().to_le_bytes_view()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the original implementation that converts data to Bytes or FixedArray[Byte] instead of BytesView.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think @Yu-zh

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to return Bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted those convertors to return Bytes.

uint/uint.mbt Outdated Show resolved Hide resolved
Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution!

@peter-jerry-ye peter-jerry-ye merged commit e687864 into moonbitlang:main Jan 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants