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

refactor buffer to use FixedArray[Byte] instead of Bytes #1231

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Nov 19, 2024

This refactor is a preparation for making Bytes immutable. It basically changes the use of Bytes in the type Buffer to FixedArray[Byte], and added Bytes::cast_from_fixedarray and cast_to_fixedarray to convert between Bytes and FixedArray[Byte]. Currently, these casts need to allocate, and we plan to implement these casts as primitives in the compiler to avoid the allocation eventually.

Copy link

peter-jerry-ye-code-review bot commented Nov 19, 2024

From the provided git diff output, here are three issues/suggestions that can be pointed out:

  1. Deprecation Alert in to_string Function:

    • The function to_string is marked as deprecated with the alert: @alert deprecated "Use Buffer::to_unchecked_string instead". This indicates that users should migrate to using to_unchecked_string instead of to_string.
    • Suggestion: Update any code that calls to_string to use to_unchecked_string to adhere to the deprecation notice and avoid potential future removal of the deprecated function.
  2. Potential Bug in grow_if_necessary Function:

    • The function grow_if_necessary checks if enough_space is less than or equal to 0 and then sets enough_space to 1. This logic might be incorrect if enough_space is exactly 0, as it implies no space is available initially.
    • Suggestion: Consider revising the condition to handle the case where enough_space is exactly 0 more clearly, possibly by initializing enough_space differently or adding a specific check for this edge case.
  3. Inconsistent Naming and Data Handling:

    • The struct T has fields renamed from bytes to data and from initial_bytes to initial_data. This change is used inconsistently across the code, with some functions still referencing the old field names (self.bytes vs. self.data).
    • Suggestion: Ensure that all references to these fields are updated to use the new names (data and initial_data) consistently throughout the code to maintain clarity and avoid potential runtime errors due to incorrect field accesses.

These observations focus on potential issues that could impact the functionality and maintainability of the code, especially concerning deprecated functions, logical correctness, and consistent naming conventions.

@coveralls
Copy link
Collaborator

coveralls commented Nov 19, 2024

Pull Request Test Coverage Report for Build 3902

Details

  • 15 of 25 (60.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 79.949%

Changes Missing Coverage Covered Lines Changed/Added Lines %
buffer/buffer.mbt 4 6 66.67%
bytes/bytes.mbt 3 6 50.0%
builtin/bytes.mbt 8 13 61.54%
Totals Coverage Status
Change from base Build 3895: 0.4%
Covered Lines: 4374
Relevant Lines: 5471

💛 - Coveralls

@Yu-zh Yu-zh requested a review from bobzhang November 19, 2024 02:57
bytes/bytes.mbt Show resolved Hide resolved
bytes/bytes.mbt Outdated Show resolved Hide resolved
bytes/bytes.mbti Show resolved Hide resolved
builtin/builtin.mbti Show resolved Hide resolved
buffer/buffer.mbt Show resolved Hide resolved
buffer/buffer.mbt Show resolved Hide resolved
bytes/bytes.mbt Outdated
/// Makes a new Bytes from a fixedarray.
pub fn Bytes::from_fixedarray(
arr : FixedArray[Byte],
len~ : Int = arr.length()
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not use this feature, we may remove the labelled dependency to keep the language simple

@bobzhang bobzhang merged commit 2688294 into moonbitlang:main Nov 20, 2024
13 checks passed
@Yoorkin Yoorkin mentioned this pull request Nov 25, 2024
5 tasks
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.

3 participants