Skip to content

Conversation

@Yu-zh
Copy link
Contributor

@Yu-zh Yu-zh commented Sep 5, 2025

This update is to accommodate moonbitlang/core#2674

@semanticdiff-com
Copy link

Review changes with  SemanticDiff

@lynzrand lynzrand force-pushed the update-template-904 branch from 140b5ce to 63f0abc Compare September 9, 2025 08:27
@peter-jerry-ye-code-review
Copy link

Inconsistent function signature for moonbit_unsafe_int_from_int across files

Category
Correctness
Code Snippet
File 1: fn[T] moonbit_unsafe_int_from_int(x : T) -> Int = "%identity"
File 2: fn moonbit_unsafe_int_from_int(x : Int) -> Int = "%identity"
Recommendation
Use consistent generic signature across all files: fn[T] moonbit_unsafe_int_from_int(x : T) -> Int = "%identity"
Reasoning
The second file uses a non-generic signature which limits type flexibility and creates inconsistency. The generic version allows for broader type conversion while maintaining the same unsafe identity operation.

Duplicated unsafe conversion function definitions across multiple files

Category
Maintainability
Code Snippet
Same function definition appears in:

  • no_args_driver_template_native.mbt
  • with_args_bench_driver_template_native.mbt
  • with_args_driver_template_native.mbt
    Recommendation
    Extract the unsafe conversion functions to a shared module or header file that can be imported by all test driver templates
    Reasoning
    Code duplication makes maintenance harder and increases the risk of inconsistencies. A shared definition ensures all templates use the same implementation and reduces the surface area for bugs.
Complex nested unsafe conversions may indicate underlying type system issues

Category
Correctness
Code Snippet
if moonbit_unsafe_char_from_int(moonbit_unsafe_int_from_int(s[i])) == sep {
Recommendation
Consider if the core library changes provide a more direct conversion function, or document why the double conversion is necessary
Reasoning
The pattern of converting from string index to int, then int to char suggests potential type system friction. This should be verified against the core library changes to ensure it's the intended approach rather than a workaround.

@lynzrand lynzrand enabled auto-merge September 9, 2025 08:27
@lynzrand lynzrand merged commit a839854 into main Sep 9, 2025
8 of 11 checks passed
@lynzrand lynzrand deleted the update-template-904 branch September 9, 2025 08: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