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 more write_X method and some bytes conversions to buffer #1307

Merged
merged 14 commits into from
Dec 11, 2024

Conversation

SyoujyoujiNaiki
Copy link
Contributor

Add methods to write UInt64, Int64, UInt, Int, Double and Float into buffer with their conversion to Bytes.

@coveralls
Copy link
Collaborator

coveralls commented Dec 8, 2024

Pull Request Test Coverage Report for Build 4190

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 80.891%

Totals Coverage Status
Change from base Build 4188: 0.04%
Covered Lines: 4504
Relevant Lines: 5568

💛 - Coveralls

buffer/buffer.mbt Outdated Show resolved Hide resolved
@Yoorkin
Copy link
Collaborator

Yoorkin commented Dec 9, 2024

Thanks for your contribution!

The write_X methods for the buffer look good to me, but the bytes_to_x seem unrelated to the buffer package. We also need further consideration about the API convention.

Can you separate out the bytes conversions part from this PR? Let's merge the write_X part first.

@Yoorkin
Copy link
Collaborator

Yoorkin commented Dec 9, 2024

Some consideration about performance:

The buffer is used to write large data consisting of multiple basic types (int, bool, double, etc.). It will call write_x frequently. We should try to eliminate the memory copy overhead in the write_x methods. Instead of converting to bytes and writing the bytes into the buffer, consider using write_byte directly

If you don't want to duplicate the code, maybe changing the implementation of X::to_bytes to use the x |> @buffer.write_x |> @buffer.to_bytes is a better choice, because those conversions are not frequently used. What do you think?

buffer/moon.pkg.json Outdated Show resolved Hide resolved
double/double.mbti Outdated Show resolved Hide resolved
@Yoorkin Yoorkin merged commit f190beb into moonbitlang:main Dec 11, 2024
13 checks passed
@SyoujyoujiNaiki SyoujyoujiNaiki deleted the buffer_changes branch December 16, 2024 06:28
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.

5 participants