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

fix: get around iter #62

Merged
merged 3 commits into from
Sep 26, 2024
Merged

fix: get around iter #62

merged 3 commits into from
Sep 26, 2024

Conversation

peter-jerry-ye
Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye commented Sep 26, 2024

This PR

  • Add test check for both debug mode and release mode
  • Get around Iter for now for a known issue

Copy link

peter-jerry-ye-code-review bot commented Sep 26, 2024

Observations and Suggestions

1. Inconsistent Function Signature in arr_u32_to_u8be

Files Affected:

  • crypto/sha224.mbt
  • crypto/sha256.mbt
  • crypto/sm3.mbt
  • crypto/utils.mbt

Issue: The function arr_u32_to_u8be is being called with an additional parameter bits in some places but not in others. This inconsistency can lead to confusion and potential runtime errors if not handled properly.

Suggestion: Ensure that the function arr_u32_to_u8be always accepts and handles the bits parameter consistently across all calls. Update the function definition in crypto/utils.mbt to reflect this change and adjust all calls accordingly.

fn arr_u32_to_u8be(x : Iter[UInt], bits : Int) -> Bytes {
  let temp : Bytes = Bytes::new(bits / 8)
  x.eachi(
    fn(i, d) {
      temp[i * 4 + 0] = d.lsr(24).to_byte()
      temp[i * 4 + 1] = d.lsr(16).to_byte()
      temp[i * 4 + 2] = d.lsr(8).to_byte()
      temp[i * 4 + 3] = d.to_byte()
    },
  )
  temp
}

2. Potential Typo in fs_test.mbt

File Affected: fs/fs_test.mbt

Issue: The content string in the inspect! block does not match the expected byte content. This could lead to confusion during debugging or testing.

Suggestion: Update the content string to match the actual byte content or ensure that the expected content is correctly represented in the test.

inspect!(
  byte,
  content=
    #|b"\x74\x61\x72\x67\x65\x74\x2f\x0a\x2e\x6d\x6f\x6f\x6e\x63\x61\x6b\x65\x73\x2f\x0a"
  ,
)

3. Redundant Conversion in fs_test.mbt

File Affected: fs/fs_test.mbt

Issue: The map function applied to byte is redundant as @fs.read_file_to_bytes!(~path) directly returns Bytes.

Suggestion: Remove the redundant map function call to simplify the code and improve readability.

let byte = @fs.read_file_to_bytes!(~path)

These changes should help maintain consistency, improve readability, and reduce potential runtime errors in the codebase.

@Yu-zh Yu-zh merged commit babcf4a into main Sep 26, 2024
8 checks passed
@Yu-zh Yu-zh deleted the zihang/get-around-iter branch September 26, 2024 02:38
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.

2 participants